From 3d357c661c5bd8c447a7a2d9678f50610d538700 Mon Sep 17 00:00:00 2001 From: Erik Ritter Date: Fri, 9 Apr 2021 09:39:02 -0700 Subject: [PATCH] feat: handle chart/data API errors (#14040) --- superset/charts/api.py | 9 +-------- superset/exceptions.py | 26 +++++++++++++++++--------- tests/charts/api_tests.py | 7 +++++++ 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/superset/charts/api.py b/superset/charts/api.py index 20ea943ee8..237b24a2b3 100644 --- a/superset/charts/api.py +++ b/superset/charts/api.py @@ -65,7 +65,7 @@ from superset.charts.schemas import ( 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 @@ class ChartRestApi(BaseSupersetModelRestApi): @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 @@ class ChartRestApi(BaseSupersetModelRestApi): "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 @@ class ChartRestApi(BaseSupersetModelRestApi): @expose("/data/", 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 @@ class ChartRestApi(BaseSupersetModelRestApi): 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 index 42e089ba45..aff21a64de 100644 --- a/superset/exceptions.py +++ b/superset/exceptions.py @@ -41,6 +41,14 @@ class SupersetException(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, @@ -48,9 +56,10 @@ class SupersetErrorException(SupersetException): 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 @@ class SupersetErrorsException(SupersetException): 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 @@ class SupersetGenericDBErrorException(SupersetErrorException): ) -class SupersetTemplateParamsErrorException(SupersetErrorException): +class SupersetTemplateParamsErrorException(SupersetErrorFromParamsException): status = 400 def __init__( @@ -94,14 +103,13 @@ class SupersetTemplateParamsErrorException(SupersetErrorException): ) -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 index 19af8c8ff5..6c9cfe1a5e 100644 --- a/tests/charts/api_tests.py +++ b/tests/charts/api_tests.py @@ -38,6 +38,7 @@ from tests.fixtures.world_bank_dashboard import load_world_bank_dashboard_with_s 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.models.slice import Slice 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 @@ class TestChartApi(SupersetTestCase, ApiOwnersTestCaseMixin, InsertChartMixin): 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):