feat: initial work to make v1 API compatible with SIP-40 and SIP-41 (#13960)

* WIP

* Use errorhandler

* Add response schema

* Fix status on HTTPException

* s/found/encountered/g

* Fix test

* Fix lint

* Fix lint and test

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 encountered 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 encountered an unexpected error.
+```
+
+Someething unexpected happened in the Superset backend. Please reach out
+to your administrator.

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> = T[keyof T];

superset/databases/api.py

@@ -41,7 +41,6 @@ from superset.databases.commands.exceptions import (
     DatabaseImportError,
     DatabaseInvalidError,
     DatabaseNotFoundError,
-    DatabaseTestConnectionFailedError,
     DatabaseUpdateFailedError,
 )
 from superset.databases.commands.export import ExportDatabasesCommand
@@ -64,7 +63,6 @@ from superset.databases.schemas import (
     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
@@ -558,7 +556,6 @@ class DatabaseRestApi(BaseSupersetModelRestApi):
 
     @expose("/test_connection", methods=["POST"])
     @protect()
-    @safe
     @statsd_metrics
     @event_logger.log_this_with_context(
         action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
@@ -604,13 +601,8 @@ class DatabaseRestApi(BaseSupersetModelRestApi):
         # This validates custom Schema with custom validations
         except ValidationError as error:
             return self.response_400(message=error.messages)
-        try:
+        TestConnectionDatabaseCommand(g.user, item).run()
-            TestConnectionDatabaseCommand(g.user, item).run()
+        return self.response(200, message="OK")
-            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)
 
     @expose("/<int:pk>/related_objects/", methods=["GET"])
     @protect()

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

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 @@ ERROR_TYPES_TO_ISSUE_CODES_MAPPING = {
             ),
         },
     ],
+    SupersetErrorType.GENERIC_COMMAND_ERROR: [
+        {
+            "code": 1010,
+            "message": _(
+                "Issue 1010 - Superset encountered an error while running a command."
+            ),
+        },
+    ],
+    SupersetErrorType.GENERIC_BACKEND_ERROR: [
+        {
+            "code": 1011,
+            "message": _("Issue 1011 - Superset encountered an unexpected error."),
+        },
+    ],
 }
 
 

superset/exceptions.py

@@ -54,6 +54,14 @@ class SupersetErrorException(SupersetException):
         )
 
 
+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 @@ class SupersetSecurityException(SupersetException):
         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

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"},
+            },
+        },
+    },
+}

superset/views/base.py

@@ -46,11 +46,13 @@ from superset import (
     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,
 )
@@ -132,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",
     )
 
 
@@ -330,6 +332,81 @@ 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
+    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(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,
+    )
+
+
+# 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,
+            ),
+        ],
+        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={},
+            ),
+        ],
+    )
+
+
 @superset_app.context_processor
 def get_common_bootstrap_data() -> Dict[str, Any]:
     def serialize_bootstrap_data() -> str:

superset/views/base_api.py

@@ -35,6 +35,7 @@ from superset.extensions import db, event_logger, security_manager
 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
@@ -77,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
 
@@ -198,6 +204,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()

tests/databases/api_tests.py

@@ -820,7 +820,21 @@ class TestDatabaseApi(SupersetTestCase):
         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 @@ class TestDatabaseApi(SupersetTestCase):
         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)
 
@@ -888,7 +916,22 @@ class TestDatabaseApi(SupersetTestCase):
         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
 
@@ -919,7 +962,23 @@ class TestDatabaseApi(SupersetTestCase):
         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
 
@@ -950,7 +1009,23 @@ class TestDatabaseApi(SupersetTestCase):
         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