From f7caae7f1fb397b367b7d6ff30bbd89f263b61cf Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Mon, 5 Apr 2021 20:02:52 -0700 Subject: [PATCH 1/8] WIP --- .../pages/docs/Miscellaneous/issue_codes.mdx | 18 +++++ .../src/components/ErrorMessage/types.ts | 8 ++- superset/databases/api.py | 16 ++--- superset/errors.py | 22 +++++- superset/exceptions.py | 14 ++-- superset/views/base_api.py | 72 ++++++++++++++++++- 6 files changed, 132 insertions(+), 18 deletions(-) diff --git a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx index 8b8e0c8e73cb1..71932bb9b5a81 100644 --- a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx +++ b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx @@ -113,3 +113,21 @@ The host might be down, and cannot be reached on the provided port. The host provided when adding a new database doesn't seem to be up. Additionally, it cannot be reached on the provided port. Please check that there are no firewall rules preventing access to the host. + +## Issue 1010 + +``` +Superset found an error while running a command. +``` + +Something unexpected happened, and Superset encountered an error while +running a command. Please reach out to your administrator. + +## Issue 1011 + +``` +Superset found an unexpected error. +``` + +Someething unexpected happened in the Superset backend. Please reach out +to your administrator. diff --git a/superset-frontend/src/components/ErrorMessage/types.ts b/superset-frontend/src/components/ErrorMessage/types.ts index 30cb2658beed2..2ffc655580224 100644 --- a/superset-frontend/src/components/ErrorMessage/types.ts +++ b/superset-frontend/src/components/ErrorMessage/types.ts @@ -28,6 +28,8 @@ export const ErrorTypeEnum = { GENERIC_DB_ENGINE_ERROR: 'GENERIC_DB_ENGINE_ERROR', COLUMN_DOES_NOT_EXIST_ERROR: 'COLUMN_DOES_NOT_EXIST_ERROR', TABLE_DOES_NOT_EXIST_ERROR: 'TABLE_DOES_NOT_EXIST_ERROR', + TEST_CONNECTION_PORT_CLOSED_ERROR: 'TEST_CONNECTION_PORT_CLOSED_ERROR', + TEST_CONNECTION_HOST_DOWN_ERROR: 'TEST_CONNECTION_HOST_DOWN_ERROR', // Viz errors VIZ_GET_DF_ERROR: 'VIZ_GET_DF_ERROR', @@ -48,8 +50,10 @@ export const ErrorTypeEnum = { MISSING_TEMPLATE_PARAMS_ERROR: 'MISSING_TEMPLATE_PARAMS_ERROR', TEST_CONNECTION_INVALID_HOSTNAME_ERROR: 'TEST_CONNECTION_INVALID_HOSTNAME_ERROR', - TEST_CONNECTION_PORT_CLOSED_ERROR: 'TEST_CONNECTION_PORT_CLOSED_ERROR', - TEST_CONNECTION_HOST_DOWN_ERROR: 'TEST_CONNECTION_HOST_DOWN_ERROR', + + // Generic errors + GENERIC_COMMAND_ERROR: 'GENERIC_COMMAND_ERROR', + GENERIC_BACKEND_ERROR: 'GENERIC_BACKEND_ERROR', } as const; type ValueOf = T[keyof T]; diff --git a/superset/databases/api.py b/superset/databases/api.py index ba43f3a09504f..e26100b7e1488 100644 --- a/superset/databases/api.py +++ b/superset/databases/api.py @@ -69,7 +69,11 @@ from superset.models.core import Database from superset.typing import FlaskResponse from superset.utils.core import error_msg_from_exception -from superset.views.base_api import BaseSupersetModelRestApi, statsd_metrics +from superset.views.base_api import ( + BaseSupersetModelRestApi, + handle_exception, + statsd_metrics, +) logger = logging.getLogger(__name__) @@ -565,6 +569,7 @@ def select_star( f".test_connection", log_to_statsd=False, ) + @handle_exception def test_connection( # pylint: disable=too-many-return-statements self, ) -> FlaskResponse: @@ -604,13 +609,8 @@ def test_connection( # pylint: disable=too-many-return-statements # This validates custom Schema with custom validations except ValidationError as error: return self.response_400(message=error.messages) - try: - TestConnectionDatabaseCommand(g.user, item).run() - return self.response(200, message="OK") - except DatabaseTestConnectionFailedError as ex: - return self.response_422(message=str(ex)) - except SupersetErrorException as ex: - return self.response(ex.status, message=ex.error.message) + TestConnectionDatabaseCommand(g.user, item).run() + return self.response(200, message="OK") @expose("//related_objects/", methods=["GET"]) @protect() diff --git a/superset/errors.py b/superset/errors.py index 248d75d27b23b..8a11a2c0efa5e 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -39,6 +39,8 @@ class SupersetErrorType(str, Enum): GENERIC_DB_ENGINE_ERROR = "GENERIC_DB_ENGINE_ERROR" COLUMN_DOES_NOT_EXIST_ERROR = "COLUMN_DOES_NOT_EXIST_ERROR" TABLE_DOES_NOT_EXIST_ERROR = "TABLE_DOES_NOT_EXIST_ERROR" + TEST_CONNECTION_PORT_CLOSED_ERROR = "TEST_CONNECTION_PORT_CLOSED_ERROR" + TEST_CONNECTION_HOST_DOWN_ERROR = "TEST_CONNECTION_HOST_DOWN_ERROR" # Viz errors VIZ_GET_DF_ERROR = "VIZ_GET_DF_ERROR" @@ -57,8 +59,10 @@ class SupersetErrorType(str, Enum): # Sql Lab errors MISSING_TEMPLATE_PARAMS_ERROR = "MISSING_TEMPLATE_PARAMS_ERROR" TEST_CONNECTION_INVALID_HOSTNAME_ERROR = "TEST_CONNECTION_INVALID_HOSTNAME_ERROR" - TEST_CONNECTION_PORT_CLOSED_ERROR = "TEST_CONNECTION_PORT_CLOSED_ERROR" - TEST_CONNECTION_HOST_DOWN_ERROR = "TEST_CONNECTION_HOST_DOWN_ERROR" + + # Generic errors + GENERIC_COMMAND_ERROR = "GENERIC_COMMAND_ERROR" + GENERIC_BACKEND_ERROR = "GENERIC_BACKEND_ERROR" ERROR_TYPES_TO_ISSUE_CODES_MAPPING = { @@ -135,6 +139,20 @@ class SupersetErrorType(str, Enum): ), }, ], + SupersetErrorType.GENERIC_COMMAND_ERROR: [ + { + "code": 1010, + "message": _( + "Issue 1010 - Superset found an error while running a command." + ), + }, + ], + SupersetErrorType.GENERIC_BACKEND_ERROR: [ + { + "code": 1011, + "message": _("Issue 1011 - Superset found an unexpected error."), + }, + ], } diff --git a/superset/exceptions.py b/superset/exceptions.py index 1d5ff2ced13d7..42e089ba45ac5 100644 --- a/superset/exceptions.py +++ b/superset/exceptions.py @@ -54,6 +54,14 @@ def __init__( ) +class SupersetErrorsException(SupersetException): + """Exceptions with multiple SupersetErrorType associated with them""" + + def __init__(self, errors: List[SupersetError]) -> None: + super().__init__(str(errors)) + self.errors = errors + + class SupersetTimeoutException(SupersetErrorException): status = 408 @@ -97,13 +105,9 @@ def __init__( self.payload = payload -class SupersetVizException(SupersetException): +class SupersetVizException(SupersetErrorsException): status = 400 - def __init__(self, errors: List[SupersetError]) -> None: - super().__init__(str(errors)) - self.errors = errors - class NoDataException(SupersetException): status = 400 diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 2956058108d66..3fd3e24b8a3da 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -31,6 +31,9 @@ from sqlalchemy import and_, distinct, func from sqlalchemy.orm.query import Query +from superset.commands.exceptions import CommandException, CommandInvalidError +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import SupersetErrorException, SupersetErrorsException from superset.extensions import db, event_logger, security_manager from superset.models.core import FavStar from superset.models.dashboard import Dashboard @@ -38,7 +41,8 @@ from superset.sql_lab import Query as SqllabQuery from superset.stats_logger import BaseStatsLogger from superset.typing import FlaskResponse -from superset.utils.core import time_function +from superset.utils.core import error_msg_from_exception, time_function +from superset.views.base import json_errors_response logger = logging.getLogger(__name__) get_related_schema = { @@ -84,6 +88,72 @@ def wraps(self: "BaseSupersetModelRestApi", *args: Any, **kwargs: Any) -> Respon return functools.update_wrapper(wraps, f) +def get_level(status: int) -> ErrorLevel: + if status < 400: + return ErrorLevel.INFO + if status < 500: + return ErrorLevel.WARNING + return ErrorLevel.ERROR + + +def handle_exception(f: Callable[..., FlaskResponse]) -> Callable[..., FlaskResponse]: + """ + A decorator that formats exceptions. + + SIP-40 (https://github.com/apache/superset/issues/9194) introduced + a standard error payload for all API errors returned by Superset. + This decorator formats exceptions to conform to it. + """ + + def wraps(self: Any, *args: Any, **kwargs: Any) -> FlaskResponse: + try: + return f(self, *args, **kwargs) + except SupersetErrorException as ex: + logger.warning(ex) + return json_errors_response(errors=[ex.error], status=ex.status) + except SupersetErrorsException as ex: + logger.warning(ex) + return json_errors_response(errors=ex.errors, status=ex.status) + except CommandInvalidError as ex: + logger.warning(ex) + return json_errors_response( + errors=[ + SupersetError( + message=ex.message, + error_type=SupersetErrorType.GENERIC_COMMAND_ERROR, + level=get_level(ex.status), + extra=ex.normalized_messages(), + ), + ] + ) + except CommandException as ex: + logger.warning(ex) + return json_errors_response( + errors=[ + SupersetError( + message=ex.message, + error_type=SupersetErrorType.GENERIC_COMMAND_ERROR, + level=get_level(ex.status), + extra={}, + ), + ] + ) + except Exception as ex: + logger.warning(ex) + return json_errors_response( + errors=[ + SupersetError( + message=error_msg_from_exception(ex), + error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, + level=ErrorLevel.ERROR, + extra={}, + ), + ] + ) + + return functools.update_wrapper(wraps, f) + + class RelatedFieldFilter: # data class to specify what filter to use on a /related endpoint # pylint: disable=too-few-public-methods From c11d9d17ea8a0e953f9ddda338f4a35f98a6c56a Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 6 Apr 2021 12:52:35 -0700 Subject: [PATCH 2/8] Use errorhandler --- superset/databases/api.py | 8 +---- superset/views/base.py | 58 ++++++++++++++++++++++++++++++ superset/views/base_api.py | 72 +------------------------------------- 3 files changed, 60 insertions(+), 78 deletions(-) diff --git a/superset/databases/api.py b/superset/databases/api.py index e26100b7e1488..6053a4274517f 100644 --- a/superset/databases/api.py +++ b/superset/databases/api.py @@ -69,11 +69,7 @@ from superset.models.core import Database from superset.typing import FlaskResponse from superset.utils.core import error_msg_from_exception -from superset.views.base_api import ( - BaseSupersetModelRestApi, - handle_exception, - statsd_metrics, -) +from superset.views.base_api import BaseSupersetModelRestApi, statsd_metrics logger = logging.getLogger(__name__) @@ -562,14 +558,12 @@ def select_star( @expose("/test_connection", methods=["POST"]) @protect() - @safe @statsd_metrics @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}" f".test_connection", log_to_statsd=False, ) - @handle_exception def test_connection( # pylint: disable=too-many-return-statements self, ) -> FlaskResponse: diff --git a/superset/views/base.py b/superset/views/base.py index b5d7138050ed5..f2dfe08333cab 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -46,11 +46,13 @@ get_feature_flags, security_manager, ) +from superset.commands.exceptions import CommandException, CommandInvalidError from superset.connectors.sqla import models from superset.datasets.commands.exceptions import get_dataset_exist_error_msg from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import ( SupersetErrorException, + SupersetErrorsException, SupersetException, SupersetSecurityException, ) @@ -330,6 +332,62 @@ def common_bootstrap_payload() -> Dict[str, Any]: } +def get_error_level_from_status_code(status: int) -> ErrorLevel: + if status < 400: + return ErrorLevel.INFO + if status < 500: + return ErrorLevel.WARNING + return ErrorLevel.ERROR + + +# SIP-40 compatible error responses; make sure APIs raise +# SupersetErrorException or SupersetErrorsException +@superset_app.errorhandler(SupersetErrorException) +def show_superset_error(ex: SupersetErrorException) -> FlaskResponse: + logger.warning(ex) + return json_errors_response(errors=[ex.error], status=ex.status) + + +@superset_app.errorhandler(SupersetErrorsException) +def show_superset_errors(ex: SupersetErrorsException) -> FlaskResponse: + logger.warning(ex) + return json_errors_response(errors=ex.errors, status=ex.status) + + +@superset_app.errorhandler(Exception) +def show_unexpected_exception(ex: Exception) -> FlaskResponse: + logger.warning(ex) + return json_errors_response( + errors=[ + SupersetError( + message=utils.error_msg_from_exception(ex), + error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, + level=ErrorLevel.ERROR, + extra={}, + ), + ], + ) + + +# Temporary handler for CommandException; if an API raises a +# CommandException it should be fixed to map it to SupersetErrorException +# or SupersetErrorsException, with a specific status code and error type +@superset_app.errorhandler(CommandException) +def show_command_errors(ex: CommandException) -> FlaskResponse: + logger.warning(ex) + extra = ex.normalized_messages() if isinstance(ex, CommandInvalidError) else {} + return json_errors_response( + errors=[ + SupersetError( + message=ex.message, + error_type=SupersetErrorType.GENERIC_COMMAND_ERROR, + level=get_error_level_from_status_code(ex.status), + extra=extra, + ), + ], + ) + + @superset_app.context_processor def get_common_bootstrap_data() -> Dict[str, Any]: def serialize_bootstrap_data() -> str: diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 3fd3e24b8a3da..2956058108d66 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -31,9 +31,6 @@ from sqlalchemy import and_, distinct, func from sqlalchemy.orm.query import Query -from superset.commands.exceptions import CommandException, CommandInvalidError -from superset.errors import ErrorLevel, SupersetError, SupersetErrorType -from superset.exceptions import SupersetErrorException, SupersetErrorsException from superset.extensions import db, event_logger, security_manager from superset.models.core import FavStar from superset.models.dashboard import Dashboard @@ -41,8 +38,7 @@ from superset.sql_lab import Query as SqllabQuery from superset.stats_logger import BaseStatsLogger from superset.typing import FlaskResponse -from superset.utils.core import error_msg_from_exception, time_function -from superset.views.base import json_errors_response +from superset.utils.core import time_function logger = logging.getLogger(__name__) get_related_schema = { @@ -88,72 +84,6 @@ def wraps(self: "BaseSupersetModelRestApi", *args: Any, **kwargs: Any) -> Respon return functools.update_wrapper(wraps, f) -def get_level(status: int) -> ErrorLevel: - if status < 400: - return ErrorLevel.INFO - if status < 500: - return ErrorLevel.WARNING - return ErrorLevel.ERROR - - -def handle_exception(f: Callable[..., FlaskResponse]) -> Callable[..., FlaskResponse]: - """ - A decorator that formats exceptions. - - SIP-40 (https://github.com/apache/superset/issues/9194) introduced - a standard error payload for all API errors returned by Superset. - This decorator formats exceptions to conform to it. - """ - - def wraps(self: Any, *args: Any, **kwargs: Any) -> FlaskResponse: - try: - return f(self, *args, **kwargs) - except SupersetErrorException as ex: - logger.warning(ex) - return json_errors_response(errors=[ex.error], status=ex.status) - except SupersetErrorsException as ex: - logger.warning(ex) - return json_errors_response(errors=ex.errors, status=ex.status) - except CommandInvalidError as ex: - logger.warning(ex) - return json_errors_response( - errors=[ - SupersetError( - message=ex.message, - error_type=SupersetErrorType.GENERIC_COMMAND_ERROR, - level=get_level(ex.status), - extra=ex.normalized_messages(), - ), - ] - ) - except CommandException as ex: - logger.warning(ex) - return json_errors_response( - errors=[ - SupersetError( - message=ex.message, - error_type=SupersetErrorType.GENERIC_COMMAND_ERROR, - level=get_level(ex.status), - extra={}, - ), - ] - ) - except Exception as ex: - logger.warning(ex) - return json_errors_response( - errors=[ - SupersetError( - message=error_msg_from_exception(ex), - error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, - level=ErrorLevel.ERROR, - extra={}, - ), - ] - ) - - return functools.update_wrapper(wraps, f) - - class RelatedFieldFilter: # data class to specify what filter to use on a /related endpoint # pylint: disable=too-few-public-methods From cded967a237622f2ec7e29c039c9cfadfb243aff Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 6 Apr 2021 13:16:02 -0700 Subject: [PATCH 3/8] Add response schema --- superset/schemas.py | 49 ++++++++++++++++++++++++++++++++++++++ superset/views/base_api.py | 13 ++++++++++ 2 files changed, 62 insertions(+) create mode 100644 superset/schemas.py diff --git a/superset/schemas.py b/superset/schemas.py new file mode 100644 index 0000000000000..52fb95f4f9151 --- /dev/null +++ b/superset/schemas.py @@ -0,0 +1,49 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +from superset.errors import SupersetErrorType + +error_payload_content = { + "application/json": { + "schema": { + "type": "object", + "properties": { + # SIP-40 error payload + "errors": { + "type": "array", + "items": { + "type": "object", + "properties": { + "message": {"type": "string"}, + "error_type": { + "type": "string", + "enum": [enum.value for enum in SupersetErrorType], + }, + "level": { + "type": "string", + "enum": ["info", "warning", "error"], + }, + "extra": {"type": "object"}, + }, + }, + }, + # Old-style message payload + "message": {"type": "string"}, + }, + }, + }, +} diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 2956058108d66..724cac957bd06 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -35,6 +35,7 @@ from superset.models.core import FavStar from superset.models.dashboard import Dashboard from superset.models.slice import Slice +from superset.schemas import error_payload_content from superset.sql_lab import Query as SqllabQuery from superset.stats_logger import BaseStatsLogger from superset.typing import FlaskResponse @@ -198,6 +199,18 @@ class BaseSupersetModelRestApi(ModelRestApi): list_columns: List[str] show_columns: List[str] + responses = { + "400": {"description": "Bad request", "content": error_payload_content}, + "401": {"description": "Unauthorized", "content": error_payload_content}, + "403": {"description": "Forbidden", "content": error_payload_content}, + "404": {"description": "Not found", "content": error_payload_content}, + "422": { + "description": "Could not process entity", + "content": error_payload_content, + }, + "500": {"description": "Fatal error", "content": error_payload_content}, + } + def __init__(self) -> None: # Setup statsd self.stats_logger = BaseStatsLogger() From 37ee2737b517aa9a2752f4018c7b4dc63dbb0ecd Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 6 Apr 2021 15:40:23 -0700 Subject: [PATCH 4/8] Fix status on HTTPException --- superset/views/base.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/superset/views/base.py b/superset/views/base.py index f2dfe08333cab..c6b00ffa1b9e5 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -354,6 +354,22 @@ def show_superset_errors(ex: SupersetErrorsException) -> FlaskResponse: return json_errors_response(errors=ex.errors, status=ex.status) +@superset_app.errorhandler(HTTPException) +def show_http_exception(ex: HTTPException) -> FlaskResponse: + logger.warning(ex) + return json_errors_response( + errors=[ + SupersetError( + message=utils.error_msg_from_exception(ex), + error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, + level=ErrorLevel.ERROR, + extra={}, + ), + ], + status=ex.code or 500, + ) + + @superset_app.errorhandler(Exception) def show_unexpected_exception(ex: Exception) -> FlaskResponse: logger.warning(ex) From 6cfd49c63ffb02d5322fbf8c789414bb186c1d7e Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 6 Apr 2021 15:45:02 -0700 Subject: [PATCH 5/8] s/found/encountered/g --- docs/src/pages/docs/Miscellaneous/issue_codes.mdx | 4 ++-- superset/errors.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx index 71932bb9b5a81..cf259ab701f3c 100644 --- a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx +++ b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx @@ -117,7 +117,7 @@ there are no firewall rules preventing access to the host. ## Issue 1010 ``` -Superset found an error while running a command. +Superset encountered an error while running a command. ``` Something unexpected happened, and Superset encountered an error while @@ -126,7 +126,7 @@ running a command. Please reach out to your administrator. ## Issue 1011 ``` -Superset found an unexpected error. +Superset encountered an unexpected error. ``` Someething unexpected happened in the Superset backend. Please reach out diff --git a/superset/errors.py b/superset/errors.py index 8a11a2c0efa5e..13cd419df960f 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -143,14 +143,14 @@ class SupersetErrorType(str, Enum): { "code": 1010, "message": _( - "Issue 1010 - Superset found an error while running a command." + "Issue 1010 - Superset encountered an error while running a command." ), }, ], SupersetErrorType.GENERIC_BACKEND_ERROR: [ { "code": 1011, - "message": _("Issue 1011 - Superset found an unexpected error."), + "message": _("Issue 1011 - Superset encountered an unexpected error."), }, ], } From 0946cfe36d1d0d5fd2853560bd6a6f55a7e1b5de Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 6 Apr 2021 17:18:05 -0700 Subject: [PATCH 6/8] Fix test --- superset/databases/commands/exceptions.py | 1 + superset/views/base.py | 34 ++++++++++++----------- superset/views/base_api.py | 7 ++++- tests/databases/api_tests.py | 32 +++++++++++++++++++-- 4 files changed, 55 insertions(+), 19 deletions(-) diff --git a/superset/databases/commands/exceptions.py b/superset/databases/commands/exceptions.py index a191ac96faa84..d2105ca962bfd 100644 --- a/superset/databases/commands/exceptions.py +++ b/superset/databases/commands/exceptions.py @@ -118,6 +118,7 @@ class DatabaseDeleteFailedReportsExistError(DatabaseDeleteFailedError): class DatabaseTestConnectionFailedError(CommandException): + status = 422 message = _("Connection failed, please check your connection settings") diff --git a/superset/views/base.py b/superset/views/base.py index c6b00ffa1b9e5..63b710bb2b75f 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -134,7 +134,7 @@ def json_errors_response( return Response( json.dumps(payload, default=utils.json_iso_dttm_ser, ignore_nan=True), status=status, - mimetype="application/json", + mimetype="application/json; charset=utf-8", ) @@ -370,21 +370,6 @@ def show_http_exception(ex: HTTPException) -> FlaskResponse: ) -@superset_app.errorhandler(Exception) -def show_unexpected_exception(ex: Exception) -> FlaskResponse: - logger.warning(ex) - return json_errors_response( - errors=[ - SupersetError( - message=utils.error_msg_from_exception(ex), - error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, - level=ErrorLevel.ERROR, - extra={}, - ), - ], - ) - - # Temporary handler for CommandException; if an API raises a # CommandException it should be fixed to map it to SupersetErrorException # or SupersetErrorsException, with a specific status code and error type @@ -401,6 +386,23 @@ def show_command_errors(ex: CommandException) -> FlaskResponse: extra=extra, ), ], + status=ex.status, + ) + + +# Catch-all, to ensure all errors from the backend conform to SIP-40 +@superset_app.errorhandler(Exception) +def show_unexpected_exception(ex: Exception) -> FlaskResponse: + logger.warning(ex) + return json_errors_response( + errors=[ + SupersetError( + message=utils.error_msg_from_exception(ex), + error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, + level=ErrorLevel.ERROR, + extra={}, + ), + ], ) diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 724cac957bd06..11632d67f2d5d 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -78,7 +78,12 @@ def statsd_metrics(f: Callable[..., Any]) -> Callable[..., Any]: """ def wraps(self: "BaseSupersetModelRestApi", *args: Any, **kwargs: Any) -> Response: - duration, response = time_function(f, self, *args, **kwargs) + try: + duration, response = time_function(f, self, *args, **kwargs) + except Exception as ex: + self.incr_stats("error", f.__name__) + raise ex + self.send_stats_metrics(response, f.__name__, duration) return response diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index 897edd0e1e7a6..7bc587070b4e1 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -820,7 +820,21 @@ def test_test_connection_failed(self): self.assertEqual(rv.headers["Content-Type"], "application/json; charset=utf-8") response = json.loads(rv.data.decode("utf-8")) expected_response = { - "message": "Could not load database driver: BaseEngineSpec", + "errors": [ + { + "message": "Could not load database driver: BaseEngineSpec", + "error_type": "GENERIC_COMMAND_ERROR", + "level": "warning", + "extra": { + "issue_codes": [ + { + "code": 1010, + "message": "Issue 1010 - Superset encountered an error while running a command.", + } + ] + }, + } + ] } self.assertEqual(response, expected_response) @@ -835,7 +849,21 @@ def test_test_connection_failed(self): self.assertEqual(rv.headers["Content-Type"], "application/json; charset=utf-8") response = json.loads(rv.data.decode("utf-8")) expected_response = { - "message": "Could not load database driver: MssqlEngineSpec", + "errors": [ + { + "message": "Could not load database driver: MssqlEngineSpec", + "error_type": "GENERIC_COMMAND_ERROR", + "level": "warning", + "extra": { + "issue_codes": [ + { + "code": 1010, + "message": "Issue 1010 - Superset encountered an error while running a command.", + } + ] + }, + } + ] } self.assertEqual(response, expected_response) From 273667f885dd9d1eae4eea8adbbe7956a6001a09 Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 6 Apr 2021 17:30:18 -0700 Subject: [PATCH 7/8] Fix lint --- superset/databases/api.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/superset/databases/api.py b/superset/databases/api.py index 6053a4274517f..970f61accf0c8 100644 --- a/superset/databases/api.py +++ b/superset/databases/api.py @@ -41,7 +41,6 @@ DatabaseImportError, DatabaseInvalidError, DatabaseNotFoundError, - DatabaseTestConnectionFailedError, DatabaseUpdateFailedError, ) from superset.databases.commands.export import ExportDatabasesCommand @@ -64,7 +63,6 @@ TableMetadataResponseSchema, ) from superset.databases.utils import get_table_metadata -from superset.exceptions import SupersetErrorException from superset.extensions import security_manager from superset.models.core import Database from superset.typing import FlaskResponse From a2d5833ea74af0d66d7e146d2b398c31a2c8870b Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 6 Apr 2021 18:37:32 -0700 Subject: [PATCH 8/8] Fix lint and test --- superset/views/base.py | 1 + tests/databases/api_tests.py | 53 ++++++++++++++++++++++++++++++++++-- 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/superset/views/base.py b/superset/views/base.py index 63b710bb2b75f..46f0d6fc6b7b0 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -332,6 +332,7 @@ def common_bootstrap_payload() -> Dict[str, Any]: } +# pylint: disable=invalid-name def get_error_level_from_status_code(status: int) -> ErrorLevel: if status < 400: return ErrorLevel.INFO diff --git a/tests/databases/api_tests.py b/tests/databases/api_tests.py index 7bc587070b4e1..174bc29f63976 100644 --- a/tests/databases/api_tests.py +++ b/tests/databases/api_tests.py @@ -916,7 +916,22 @@ def test_test_connection_failed_invalid_hostname(self, mock_is_hostname_valid): assert rv.headers["Content-Type"] == "application/json; charset=utf-8" response = json.loads(rv.data.decode("utf-8")) expected_response = { - "message": 'Unable to resolve hostname "invalidhostname".', + "errors": [ + { + "message": 'Unable to resolve hostname "invalidhostname".', + "error_type": "TEST_CONNECTION_INVALID_HOSTNAME_ERROR", + "level": "error", + "extra": { + "hostname": "invalidhostname", + "issue_codes": [ + { + "code": 1007, + "message": "Issue 1007 - The hostname provided can't be resolved.", + } + ], + }, + } + ] } assert response == expected_response @@ -947,7 +962,23 @@ def test_test_connection_failed_closed_port( assert rv.headers["Content-Type"] == "application/json; charset=utf-8" response = json.loads(rv.data.decode("utf-8")) expected_response = { - "message": "The host localhost is up, but the port 12345 is closed.", + "errors": [ + { + "message": "The host localhost is up, but the port 12345 is closed.", + "error_type": "TEST_CONNECTION_PORT_CLOSED_ERROR", + "level": "error", + "extra": { + "hostname": "localhost", + "port": 12345, + "issue_codes": [ + { + "code": 1008, + "message": "Issue 1008 - The port is closed.", + } + ], + }, + } + ] } assert response == expected_response @@ -978,7 +1009,23 @@ def test_test_connection_failed_host_down( assert rv.headers["Content-Type"] == "application/json; charset=utf-8" response = json.loads(rv.data.decode("utf-8")) expected_response = { - "message": "The host localhost might be down, ond can't be reached on port 12345.", + "errors": [ + { + "message": "The host localhost might be down, ond can't be reached on port 12345.", + "error_type": "TEST_CONNECTION_HOST_DOWN_ERROR", + "level": "error", + "extra": { + "hostname": "localhost", + "port": 12345, + "issue_codes": [ + { + "code": 1009, + "message": "Issue 1009 - The host might be down, and can't be reached on the provided port.", + } + ], + }, + } + ] } assert response == expected_response