feat: handle chart/data API errors (apache#14040)

eran-levy · Apr 9, 2021 · 3d357c6 · 3d357c6
1 parent ec3f8d0
commit 3d357c6
Show file tree

Hide file tree

Showing 3 changed files with 25 additions and 17 deletions.
diff --git a/superset/charts/api.py b/superset/charts/api.py
@@ -65,7 +65,7 @@
 from superset.commands.exceptions import CommandInvalidError
 from superset.commands.importers.v1.utils import get_contents_from_bundle
 from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod
-from superset.exceptions import QueryObjectValidationError, SupersetSecurityException
+from superset.exceptions import QueryObjectValidationError
 from superset.extensions import event_logger
 from superset.models.slice import Slice
 from superset.tasks.thumbnails import cache_chart_thumbnail
@@ -500,7 +500,6 @@ def get_data_response(
 
     @expose("/data", methods=["POST"])
     @protect()
-    @safe
     @statsd_metrics
     @event_logger.log_this_with_context(
         action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.data",
@@ -569,8 +568,6 @@ def data(self) -> Response:
                     "Request is incorrect: %(error)s", error=error.normalized_messages()
                 )
             )
-        except SupersetSecurityException:
-            return self.response_401()
 
         # TODO: support CSV, SQL query and other non-JSON types
         if (
@@ -591,7 +588,6 @@ def data(self) -> Response:
 
     @expose("/data/<cache_key>", methods=["GET"])
     @protect()
-    @safe
     @statsd_metrics
     @event_logger.log_this_with_context(
         action=lambda self, *args, **kwargs: f"{self.__class__.__name__}"
@@ -641,9 +637,6 @@ def data_from_cache(self, cache_key: str) -> Response:
             return self.response_400(
                 message=_("Request is incorrect: %(error)s", error=error.messages)
             )
-        except SupersetSecurityException as exc:
-            logger.info(exc)
-            return self.response_401()
 
         return self.get_data_response(command, True)
 

diff --git a/superset/exceptions.py b/superset/exceptions.py
@@ -41,16 +41,25 @@ def exception(self) -> Optional[Exception]:
 class SupersetErrorException(SupersetException):
     """Exceptions with a single SupersetErrorType associated with them"""
 
+    def __init__(self, error: SupersetError) -> None:
+        super().__init__(error.message)
+        self.error = error
+
+
+class SupersetErrorFromParamsException(SupersetErrorException):
+    """Exceptions that pass in parameters to construct a SupersetError"""
+
     def __init__(
         self,
         error_type: SupersetErrorType,
         message: str,
         level: ErrorLevel,
         extra: Optional[Dict[str, Any]] = None,
     ) -> None:
-        super().__init__(message)
-        self.error = SupersetError(
-            error_type=error_type, message=message, level=level, extra=extra or {}
+        super().__init__(
+            SupersetError(
+                error_type=error_type, message=message, level=level, extra=extra or {}
+            )
         )
 
 
@@ -62,11 +71,11 @@ def __init__(self, errors: List[SupersetError]) -> None:
         self.errors = errors
 
 
-class SupersetTimeoutException(SupersetErrorException):
+class SupersetTimeoutException(SupersetErrorFromParamsException):
     status = 408
 
 
-class SupersetGenericDBErrorException(SupersetErrorException):
+class SupersetGenericDBErrorException(SupersetErrorFromParamsException):
     status = 400
 
     def __init__(
@@ -80,7 +89,7 @@ def __init__(
         )
 
 
-class SupersetTemplateParamsErrorException(SupersetErrorException):
+class SupersetTemplateParamsErrorException(SupersetErrorFromParamsException):
     status = 400
 
     def __init__(
@@ -94,14 +103,13 @@ def __init__(
         )
 
 
-class SupersetSecurityException(SupersetException):
+class SupersetSecurityException(SupersetErrorException):
     status = 401
 
     def __init__(
         self, error: SupersetError, payload: Optional[Dict[str, Any]] = None
     ) -> None:
-        super().__init__(error.message)
-        self.error = error
+        super().__init__(error)
         self.payload = payload
 
 

diff --git a/tests/charts/api_tests.py b/tests/charts/api_tests.py
@@ -38,6 +38,7 @@
 from tests.test_app import app
 from superset.charts.commands.data import ChartDataCommand
 from superset.connectors.sqla.models import SqlaTable, TableColumn
+from superset.errors import SupersetErrorType
 from superset.extensions import async_query_manager, cache_manager, db
 from superset.models.annotations import AnnotationLayer
 from superset.models.core import Database, FavStar, FavStarClassName
@@ -47,6 +48,7 @@
 from superset.utils import core as utils
 from superset.utils.core import AnnotationType, get_example_database, get_main_database
 
+
 from tests.base_api_tests import ApiOwnersTestCaseMixin
 from tests.base_tests import SupersetTestCase, post_assert_metric, test_client
 
@@ -1345,6 +1347,11 @@ def test_query_exec_not_allowed(self):
         payload = get_query_context("birth_names")
         rv = self.post_assert_metric(CHART_DATA_URI, payload, "data")
         self.assertEqual(rv.status_code, 401)
+        response_payload = json.loads(rv.data.decode("utf-8"))
+        assert (
+            response_payload["errors"][0]["error_type"]
+            == SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR
+        )
 
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")
     def test_chart_data_jinja_filter_request(self):