From b57b6c778316302444bae388d0b4c74cfc9acf3a Mon Sep 17 00:00:00 2001 From: Elizabeth Thompson Date: Mon, 23 Sep 2024 17:50:48 -0700 Subject: [PATCH] add logging for error handling apis --- superset/commands/database/exceptions.py | 12 +++++++++ superset/dashboards/api.py | 14 +++-------- superset/views/error_handling.py | 5 ++-- .../integration_tests/dashboards/api_tests.py | 25 +++++++++++++++++-- 4 files changed, 42 insertions(+), 14 deletions(-) diff --git a/superset/commands/database/exceptions.py b/superset/commands/database/exceptions.py index 410eb9236b210..216252c70d83a 100644 --- a/superset/commands/database/exceptions.py +++ b/superset/commands/database/exceptions.py @@ -202,3 +202,15 @@ class DatabaseOfflineError(SupersetErrorException): class InvalidParametersError(SupersetErrorsException): status = 422 + + +class DatasetValidationError(CommandException): + status = 422 + + def __init__(self, err: Exception) -> None: + super().__init__( + _( + "Dataset schema is invalid, caused by: %(error)s", + error=str(err), + ) + ) diff --git a/superset/dashboards/api.py b/superset/dashboards/api.py index 9076303b20cf1..a4defe5c528a1 100644 --- a/superset/dashboards/api.py +++ b/superset/dashboards/api.py @@ -56,6 +56,7 @@ from superset.commands.dashboard.permalink.create import CreateDashboardPermalinkCommand from superset.commands.dashboard.unfave import DelFavoriteDashboardCommand from superset.commands.dashboard.update import UpdateDashboardCommand +from superset.commands.database.exceptions import DatasetValidationError from superset.commands.exceptions import TagForbiddenError from superset.commands.importers.exceptions import NoValidFilesFoundError from superset.commands.importers.v1.utils import get_contents_from_bundle @@ -115,6 +116,7 @@ requires_json, statsd_metrics, ) +from superset.views.error_handling import handle_api_exception from superset.views.filters import ( BaseFilterRelatedRoles, BaseFilterRelatedUsers, @@ -369,7 +371,7 @@ def get( @expose("//datasets", methods=("GET",)) @protect() - @safe + @handle_api_exception @statsd_metrics @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.get_datasets", @@ -417,15 +419,7 @@ def get_datasets(self, id_or_slug: str) -> Response: ] return self.response(200, result=result) except (TypeError, ValueError) as err: - return self.response_400( - message=gettext( - "Dataset schema is invalid, caused by: %(error)s", error=str(err) - ) - ) - except DashboardAccessDeniedError: - return self.response_403() - except DashboardNotFoundError: - return self.response_404() + raise DatasetValidationError(err) from err @expose("//tabs", methods=("GET",)) @protect() diff --git a/superset/views/error_handling.py b/superset/views/error_handling.py index b9d0a410861b4..2c538b4340d9e 100644 --- a/superset/views/error_handling.py +++ b/superset/views/error_handling.py @@ -45,6 +45,7 @@ ) from superset.superset_typing import FlaskResponse from superset.utils import core as utils, json +from superset.utils.log import get_logger_from_status if typing.TYPE_CHECKING: from superset.views.base import BaseSupersetView @@ -108,8 +109,8 @@ def wraps(self: BaseSupersetView, *args: Any, **kwargs: Any) -> FlaskResponse: logger.warning("SupersetErrorException", exc_info=True) return json_error_response([ex.error], status=ex.status) except SupersetException as ex: - if ex.status >= 500: - logger.exception(ex) + logger_func, _ = get_logger_from_status(ex.status) + logger_func(ex.message, exc_info=True) return json_error_response( utils.error_msg_from_exception(ex), status=ex.status ) diff --git a/tests/integration_tests/dashboards/api_tests.py b/tests/integration_tests/dashboards/api_tests.py index 86a855a289073..e1f3d457a4208 100644 --- a/tests/integration_tests/dashboards/api_tests.py +++ b/tests/integration_tests/dashboards/api_tests.py @@ -270,7 +270,8 @@ def create_dashboards_some_with_tags(self, create_custom_tags): # noqa: F811 db.session.commit() @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") - def test_get_dashboard_datasets(self): + @patch("superset.utils.log.logger") + def test_get_dashboard_datasets(self, logger_mock): self.login(ADMIN_USERNAME) uri = "api/v1/dashboard/world_health/datasets" response = self.get_assert_metric(uri, "get_datasets") @@ -283,6 +284,7 @@ def test_get_dashboard_datasets(self): self.assertEqual(actual_dataset_ids, expected_dataset_ids) expected_values = [0, 1] if backend() == "presto" else [0, 1, 2] self.assertEqual(result[0]["column_types"], expected_values) + logger_mock.warning.assert_not_called() @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") @patch("superset.dashboards.schemas.security_manager.has_guest_access") @@ -303,11 +305,30 @@ def test_get_dashboard_datasets_as_guest(self, is_guest_user, has_guest_access): assert excluded_key not in dataset @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") - def test_get_dashboard_datasets_not_found(self): + @patch("superset.utils.log.logger") + def test_get_dashboard_datasets_not_found(self, logger_mock): self.login(ALPHA_USERNAME) uri = "api/v1/dashboard/not_found/datasets" response = self.get_assert_metric(uri, "get_datasets") self.assertEqual(response.status_code, 404) + logger_mock.warning.assert_called_once_with( + "Dashboard not found.", exc_info=True + ) + + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + @patch("superset.utils.log.logger") + @patch("superset.daos.dashboard.DashboardDAO.get_datasets_for_dashboard") + def test_get_dashboard_datasets_invalid_schema( + self, dashboard_datasets_mock, logger_mock + ): + dashboard_datasets_mock.side_effect = TypeError("Invalid schema") + self.login(ADMIN_USERNAME) + uri = "api/v1/dashboard/world_health/datasets" + response = self.get_assert_metric(uri, "get_datasets") + self.assertEqual(response.status_code, 422) + logger_mock.warning.assert_called_once_with( + "Dataset schema is invalid, caused by: Invalid schema", exc_info=True + ) @pytest.mark.usefixtures("create_dashboards") def test_get_gamma_dashboard_datasets(self):