[POC] SIP-41 global error messages

superset/app.py

@@ -26,6 +26,7 @@
 from flask_wtf import CSRFProtect
 
 from superset.connectors.connector_registry import ConnectorRegistry
+from superset.errors import init_global_error_handlers
 from superset.extensions import (
     _event_logger,
     APP_DIR,
@@ -454,6 +455,7 @@ def init_app_in_ctx(self) -> None:
         if flask_app_mutator:
             flask_app_mutator(self.flask_app)
 
+        init_global_error_handlers(self.flask_app)
         self.init_views()
 
     def init_app(self) -> None:

superset/commands/exceptions.py

@@ -69,7 +69,7 @@ class DeleteFailedError(CommandException):
 
 
 class ForbiddenError(CommandException):
-    status = 500
+    status = 403
     message = "Action is forbidden"
 
 

superset/dashboards/api.py

@@ -18,23 +18,14 @@
 from typing import Any
 
 from flask import g, make_response, request, Response
-from flask_appbuilder.api import expose, protect, rison, safe
+from flask_appbuilder.api import expose, protect, rison
 from flask_appbuilder.models.sqla.interface import SQLAInterface
 from flask_babel import ngettext
 
 from superset.constants import RouteMethod
 from superset.dashboards.commands.bulk_delete import BulkDeleteDashboardCommand
 from superset.dashboards.commands.create import CreateDashboardCommand
 from superset.dashboards.commands.delete import DeleteDashboardCommand
-from superset.dashboards.commands.exceptions import (
-    DashboardBulkDeleteFailedError,
-    DashboardCreateFailedError,
-    DashboardDeleteFailedError,
-    DashboardForbiddenError,
-    DashboardInvalidError,
-    DashboardNotFoundError,
-    DashboardUpdateFailedError,
-)
 from superset.dashboards.commands.update import UpdateDashboardCommand
 from superset.dashboards.filters import DashboardFilter, DashboardTitleOrSlugFilter
 from superset.dashboards.schemas import (
@@ -125,7 +116,6 @@ class DashboardRestApi(BaseSupersetModelRestApi):
 
     @expose("/", methods=["POST"])
     @protect()
-    @safe
     def post(self) -> Response:
         """Creates a new Dashboard
         ---
@@ -166,18 +156,11 @@ def post(self) -> Response:
         # This validates custom Schema with custom validations
         if item.errors:
             return self.response_400(message=item.errors)
-        try:
-            new_model = CreateDashboardCommand(g.user, item.data).run()
-            return self.response(201, id=new_model.id, result=item.data)
-        except DashboardInvalidError as ex:
-            return self.response_422(message=ex.normalized_messages())
-        except DashboardCreateFailedError as ex:
-            logger.error(f"Error creating model {self.__class__.__name__}: {ex}")
-            return self.response_422(message=str(ex))
+        new_model = CreateDashboardCommand(g.user, item.data).run()
+        return self.response(201, id=new_model.id, result=item.data)
 
     @expose("/<pk>", methods=["PUT"])
     @protect()
-    @safe
     def put(  # pylint: disable=too-many-return-statements, arguments-differ
         self, pk: int
     ) -> Response:
@@ -229,22 +212,11 @@ def put(  # pylint: disable=too-many-return-statements, arguments-differ
         # This validates custom Schema with custom validations
         if item.errors:
             return self.response_400(message=item.errors)
-        try:
-            changed_model = UpdateDashboardCommand(g.user, pk, item.data).run()
-            return self.response(200, id=changed_model.id, result=item.data)
-        except DashboardNotFoundError:
-            return self.response_404()
-        except DashboardForbiddenError:
-            return self.response_403()
-        except DashboardInvalidError as ex:
-            return self.response_422(message=ex.normalized_messages())
-        except DashboardUpdateFailedError as ex:
-            logger.error(f"Error updating model {self.__class__.__name__}: {ex}")
-            return self.response_422(message=str(ex))
+        changed_model = UpdateDashboardCommand(g.user, pk, item.data).run()
+        return self.response(200, id=changed_model.id, result=item.data)
 
     @expose("/<pk>", methods=["DELETE"])
     @protect()
-    @safe
     def delete(self, pk: int) -> Response:  # pylint: disable=arguments-differ
         """Deletes a Dashboard
         ---
@@ -277,20 +249,11 @@ def delete(self, pk: int) -> Response:  # pylint: disable=arguments-differ
             500:
               $ref: '#/components/responses/500'
         """
-        try:
-            DeleteDashboardCommand(g.user, pk).run()
-            return self.response(200, message="OK")
-        except DashboardNotFoundError:
-            return self.response_404()
-        except DashboardForbiddenError:
-            return self.response_403()
-        except DashboardDeleteFailedError as ex:
-            logger.error(f"Error deleting model {self.__class__.__name__}: {ex}")
-            return self.response_422(message=str(ex))
+        DeleteDashboardCommand(g.user, pk).run()
+        return self.response(200, message="OK")
 
     @expose("/", methods=["DELETE"])
     @protect()
-    @safe
     @rison(get_delete_ids_schema)
     def bulk_delete(
         self, **kwargs: Any
@@ -331,26 +294,18 @@ def bulk_delete(
               $ref: '#/components/responses/500'
         """
         item_ids = kwargs["rison"]
-        try:
-            BulkDeleteDashboardCommand(g.user, item_ids).run()
-            return self.response(
-                200,
-                message=ngettext(
-                    f"Deleted %(num)d dashboard",
-                    f"Deleted %(num)d dashboards",
-                    num=len(item_ids),
-                ),
-            )
-        except DashboardNotFoundError:
-            return self.response_404()
-        except DashboardForbiddenError:
-            return self.response_403()
-        except DashboardBulkDeleteFailedError as ex:
-            return self.response_422(message=str(ex))
+        BulkDeleteDashboardCommand(g.user, item_ids).run()
+        return self.response(
+            200,
+            message=ngettext(
+                f"Deleted %(num)d dashboard",
+                f"Deleted %(num)d dashboards",
+                num=len(item_ids),
+            ),
+        )
 
     @expose("/export/", methods=["GET"])
     @protect()
-    @safe
     @rison(get_export_ids_schema)
     def export(self, **kwargs: Any) -> Response:
         """Export dashboards

superset/dashboards/commands/exceptions.py

@@ -41,6 +41,7 @@ class DashboardInvalidError(CommandInvalidError):
 
 
 class DashboardNotFoundError(CommandException):
+    status = 404
     message = _("Dashboard not found.")
 
 

superset/errors.py

@@ -0,0 +1,47 @@
+# 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 flask import Flask, jsonify, make_response, Response
+
+from superset.commands.exceptions import CommandInvalidError
+from superset.exceptions import SupersetException
+
+
+def init_global_error_handlers(app: Flask) -> None:
+    def error_response(code, **kwargs) -> Response:
+        """
+            Generic HTTP JSON response method
+
+        :param code: HTTP code (int)
+        :param kwargs: Data structure for response (dict)
+        :return: HTTP Json response
+        """
+        _ret_json = jsonify(kwargs)
+        resp = make_response(_ret_json, code)
+        resp.headers["Content-Type"] = "application/json; charset=utf-8"
+        return resp
+
+    @app.errorhandler(CommandInvalidError)
+    def command_invalid_error(ex):  # pylint: disable=unused-variable
+        return error_response(ex.status, message=ex.normalized_messages())
+
+    @app.errorhandler(SupersetException)
+    def superset_generic_error(ex):  # pylint: disable=unused-variable
+        return error_response(ex.status, message=str(ex))
+
+    @app.errorhandler(Exception)
+    def generic_error(ex):  # pylint: disable=unused-variable
+        return error_response(500, message=str(ex))

tests/dashboards/api_tests.py

@@ -420,7 +420,7 @@ def test_delete_bulk_dashboard_not_owned(self):
         rv = self.client.delete(uri)
         self.assertEqual(rv.status_code, 403)
         response = json.loads(rv.data.decode("utf-8"))
-        expected_response = {"message": "Forbidden"}
+        expected_response = {"message": "Changing this Dashboard is forbidden"}
         self.assertEqual(response, expected_response)
 
         # nothing is deleted in bulk with a list of owned and not owned dashboards
@@ -429,7 +429,7 @@ def test_delete_bulk_dashboard_not_owned(self):
         rv = self.client.delete(uri)
         self.assertEqual(rv.status_code, 403)
         response = json.loads(rv.data.decode("utf-8"))
-        expected_response = {"message": "Forbidden"}
+        expected_response = {"message": "Changing this Dashboard is forbidden"}
         self.assertEqual(response, expected_response)
 
         for dashboard in dashboards: