From e749efcb970a41d8e6282a7cb0a92e4f68453da2 Mon Sep 17 00:00:00 2001 From: Maxime Beauchemin Date: Tue, 9 Jul 2024 10:16:40 -0700 Subject: [PATCH] fix: refactor view error handling into a separate module (#29330) --- superset/databases/api.py | 4 +- superset/initialization/__init__.py | 3 + superset/utils/core.py | 2 +- superset/views/api.py | 3 +- superset/views/base.py | 192 +----------------------- superset/views/base_api.py | 2 +- superset/views/core.py | 12 +- superset/views/datasource/views.py | 2 +- superset/views/error_handling.py | 222 ++++++++++++++++++++++++++++ 9 files changed, 236 insertions(+), 206 deletions(-) create mode 100644 superset/views/error_handling.py diff --git a/superset/databases/api.py b/superset/databases/api.py index 3a672eb76..af5ce255a 100644 --- a/superset/databases/api.py +++ b/superset/databases/api.py @@ -123,13 +123,13 @@ from superset.utils import json from superset.utils.core import error_msg_from_exception, parse_js_uri_path_item from superset.utils.oauth2 import decode_oauth2_state from superset.utils.ssh_tunnel import mask_password_info -from superset.views.base import json_errors_response from superset.views.base_api import ( BaseSupersetModelRestApi, requires_form_data, requires_json, statsd_metrics, ) +from superset.views.error_handling import json_error_response logger = logging.getLogger(__name__) @@ -458,7 +458,7 @@ class DatabaseRestApi(BaseSupersetModelRestApi): except DatabaseConnectionFailedError as ex: return self.response_422(message=str(ex)) except SupersetErrorsException as ex: - return json_errors_response(errors=ex.errors, status=ex.status) + return json_error_response(ex.errors, status=ex.status) except DatabaseCreateFailedError as ex: logger.error( "Error creating model %s: %s", diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py index f074eaf29..7c35fd17a 100644 --- a/superset/initialization/__init__.py +++ b/superset/initialization/__init__.py @@ -174,6 +174,7 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods from superset.views.database.views import DatabaseView from superset.views.datasource.views import DatasetEditor, Datasource from superset.views.dynamic_plugins import DynamicPluginsView + from superset.views.error_handling import set_app_error_handlers from superset.views.explore import ExplorePermalinkView, ExploreView from superset.views.key_value import KV from superset.views.log.api import LogRestApi @@ -188,6 +189,8 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods from superset.views.tags import TagModelView, TagView from superset.views.users.api import CurrentUserRestApi, UserRestApi + set_app_error_handlers(self.superset_app) + # # Setup API views # diff --git a/superset/utils/core.py b/superset/utils/core.py index d01dd517e..b2f09aac2 100644 --- a/superset/utils/core.py +++ b/superset/utils/core.py @@ -433,7 +433,7 @@ def error_msg_from_exception(ex: Exception) -> str: msg = ex.message.get("message") # type: ignore elif ex.message: msg = ex.message - return msg or str(ex) + return str(msg) or str(ex) def markdown(raw: str, markup_wrap: bool | None = False) -> str: diff --git a/superset/views/api.py b/superset/views/api.py index 8ef59cb48..736c0b554 100644 --- a/superset/views/api.py +++ b/superset/views/api.py @@ -34,7 +34,8 @@ from superset.models.slice import Slice from superset.superset_typing import FlaskResponse from superset.utils import json from superset.utils.date_parser import get_since_until -from superset.views.base import api, BaseSupersetView, handle_api_exception +from superset.views.base import api, BaseSupersetView +from superset.views.error_handling import handle_api_exception if TYPE_CHECKING: from superset.common.query_context_factory import QueryContextFactory diff --git a/superset/views/base.py b/superset/views/base.py index b836c5076..f809d616c 100644 --- a/superset/views/base.py +++ b/superset/views/base.py @@ -16,14 +16,12 @@ # under the License. from __future__ import annotations -import dataclasses import functools import logging import os import traceback from datetime import datetime -from importlib.resources import files -from typing import Any, Callable, cast +from typing import Any, Callable import yaml from babel import Locale @@ -33,9 +31,7 @@ from flask import ( g, get_flashed_messages, redirect, - request, Response, - send_file, session, ) from flask_appbuilder import BaseView, expose, Model, ModelView @@ -52,11 +48,8 @@ from flask_appbuilder.security.sqla.models import User from flask_appbuilder.widgets import ListWidget from flask_babel import get_locale, gettext as __ from flask_jwt_extended.exceptions import NoAuthorizationError -from flask_wtf.csrf import CSRFError from flask_wtf.form import FlaskForm -from sqlalchemy import exc from sqlalchemy.orm import Query -from werkzeug.exceptions import HTTPException from wtforms.fields.core import Field, UnboundField from superset import ( @@ -68,17 +61,9 @@ from superset import ( is_feature_enabled, security_manager, ) -from superset.commands.exceptions import CommandException, CommandInvalidError from superset.connectors.sqla import models from superset.db_engine_specs import get_available_engine_specs from superset.db_engine_specs.gsheets import GSheetsEngineSpec -from superset.errors import ErrorLevel, SupersetError, SupersetErrorType -from superset.exceptions import ( - SupersetErrorException, - SupersetErrorsException, - SupersetException, - SupersetSecurityException, -) from superset.extensions import cache_manager from superset.models.helpers import ImportExportMixin from superset.reports.models import ReportRecipientType @@ -86,6 +71,7 @@ from superset.superset_typing import FlaskResponse from superset.translations.utils import get_language_pack from superset.utils import core as utils, json from superset.utils.filters import get_dataset_access_filters +from superset.views.error_handling import json_error_response from .utils import bootstrap_user_data @@ -146,35 +132,6 @@ def get_error_msg() -> str: return error_msg -def json_error_response( - msg: str | None = None, - status: int = 500, - payload: dict[str, Any] | None = None, -) -> FlaskResponse: - payload = payload or {"error": f"{msg}"} - - return Response( - json.dumps(payload, default=json.json_iso_dttm_ser, ignore_nan=True), - status=status, - mimetype="application/json", - ) - - -def json_errors_response( - errors: list[SupersetError], - status: int = 500, - payload: dict[str, Any] | None = None, -) -> FlaskResponse: - payload = payload or {} - - payload["errors"] = [dataclasses.asdict(error) for error in errors] - return Response( - json.dumps(payload, default=json.json_iso_dttm_ser, ignore_nan=True), - status=status, - mimetype="application/json; charset=utf-8", - ) - - def json_success(json_msg: str, status: int = 200) -> FlaskResponse: return Response(json_msg, status=status, mimetype="application/json") @@ -243,50 +200,6 @@ def api(f: Callable[..., FlaskResponse]) -> Callable[..., FlaskResponse]: return functools.update_wrapper(wraps, f) -def handle_api_exception( - f: Callable[..., FlaskResponse], -) -> Callable[..., FlaskResponse]: - """ - A decorator to catch superset exceptions. Use it after the @api decorator above - so superset exception handler is triggered before the handler for generic - exceptions. - """ - - def wraps(self: BaseSupersetView, *args: Any, **kwargs: Any) -> FlaskResponse: - try: - return f(self, *args, **kwargs) - except SupersetSecurityException as ex: - logger.warning("SupersetSecurityException", exc_info=True) - return json_errors_response( - errors=[ex.error], status=ex.status, payload=ex.payload - ) - except SupersetErrorsException as ex: - logger.warning(ex, exc_info=True) - return json_errors_response(errors=ex.errors, status=ex.status) - except SupersetErrorException as ex: - logger.warning("SupersetErrorException", exc_info=True) - return json_errors_response(errors=[ex.error], status=ex.status) - except SupersetException as ex: - if ex.status >= 500: - logger.exception(ex) - return json_error_response( - utils.error_msg_from_exception(ex), status=ex.status - ) - except HTTPException as ex: - logger.exception(ex) - return json_error_response( - utils.error_msg_from_exception(ex), status=cast(int, ex.code) - ) - except (exc.IntegrityError, exc.DatabaseError, exc.DataError) as ex: - logger.exception(ex) - return json_error_response(utils.error_msg_from_exception(ex), status=422) - except Exception as ex: # pylint: disable=broad-except - logger.exception(ex) - return json_error_response(utils.error_msg_from_exception(ex)) - - return functools.update_wrapper(wraps, f) - - class BaseSupersetView(BaseView): @staticmethod def json_response(obj: Any, status: int = 200) -> FlaskResponse: @@ -439,107 +352,6 @@ def common_bootstrap_payload() -> dict[str, Any]: } -def get_error_level_from_status_code( # pylint: disable=invalid-name - 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("SupersetErrorException", exc_info=True) - return json_errors_response(errors=[ex.error], status=ex.status) - - -@superset_app.errorhandler(SupersetErrorsException) -def show_superset_errors(ex: SupersetErrorsException) -> FlaskResponse: - logger.warning("SupersetErrorsException", exc_info=True) - return json_errors_response(errors=ex.errors, status=ex.status) - - -# Redirect to login if the CSRF token is expired -@superset_app.errorhandler(CSRFError) -def refresh_csrf_token(ex: CSRFError) -> FlaskResponse: - logger.warning("Refresh CSRF token error", exc_info=True) - - if request.is_json: - return show_http_exception(ex) - - return redirect(appbuilder.get_url_for_login) - - -@superset_app.errorhandler(HTTPException) -def show_http_exception(ex: HTTPException) -> FlaskResponse: - logger.warning("HTTPException", exc_info=True) - if ( - "text/html" in request.accept_mimetypes - and not config["DEBUG"] - and ex.code in {404, 500} - ): - path = files("superset") / f"static/assets/{ex.code}.html" - return send_file(path, max_age=0), ex.code - - return json_errors_response( - errors=[ - SupersetError( - message=utils.error_msg_from_exception(ex), - error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, - level=ErrorLevel.ERROR, - ), - ], - 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("CommandException", exc_info=True) - if "text/html" in request.accept_mimetypes and not config["DEBUG"]: - path = files("superset") / "static/assets/500.html" - return send_file(path, max_age=0), 500 - - 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.exception(ex) - if "text/html" in request.accept_mimetypes and not config["DEBUG"]: - path = files("superset") / "static/assets/500.html" - return send_file(path, max_age=0), 500 - - return json_errors_response( - errors=[ - SupersetError( - message=utils.error_msg_from_exception(ex), - error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, - level=ErrorLevel.ERROR, - ), - ], - ) - - @superset_app.context_processor def get_common_bootstrap_data() -> dict[str, Any]: def serialize_bootstrap_data() -> str: diff --git a/superset/views/base_api.py b/superset/views/base_api.py index 9436ce639..5c7114751 100644 --- a/superset/views/base_api.py +++ b/superset/views/base_api.py @@ -42,7 +42,7 @@ from superset.sql_lab import Query as SqllabQuery from superset.superset_typing import FlaskResponse from superset.tags.models import Tag from superset.utils.core import get_user_id, time_function -from superset.views.base import handle_api_exception +from superset.views.error_handling import handle_api_exception logger = logging.getLogger(__name__) get_related_schema = { diff --git a/superset/views/core.py b/superset/views/core.py index 75a04dedc..56221b646 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -23,7 +23,7 @@ from datetime import datetime from typing import Any, Callable, cast from urllib import parse -from flask import abort, flash, g, redirect, render_template, request, Response +from flask import abort, flash, g, redirect, request, Response from flask_appbuilder import expose from flask_appbuilder.security.decorators import ( has_access, @@ -85,11 +85,10 @@ from superset.views.base import ( data_payload_response, deprecated, generate_download_headers, - get_error_msg, - handle_api_exception, json_error_response, json_success, ) +from superset.views.error_handling import handle_api_exception from superset.views.utils import ( bootstrap_user_data, check_datasource_perms, @@ -888,13 +887,6 @@ class Superset(BaseSupersetView): datasource.raise_for_access() return json_success(json.dumps(sanitize_datasource_data(datasource.data))) - @app.errorhandler(500) - def show_traceback(self) -> FlaskResponse: - return ( - render_template("superset/traceback.html", error_msg=get_error_msg()), - 500, - ) - @event_logger.log_this @expose("/welcome/") def welcome(self) -> FlaskResponse: diff --git a/superset/views/datasource/views.py b/superset/views/datasource/views.py index 377579cf0..fd0dc2590 100644 --- a/superset/views/datasource/views.py +++ b/superset/views/datasource/views.py @@ -44,7 +44,6 @@ from superset.views.base import ( api, BaseSupersetView, deprecated, - handle_api_exception, json_error_response, ) from superset.views.datasource.schemas import ( @@ -55,6 +54,7 @@ from superset.views.datasource.schemas import ( SamplesRequestSchema, ) from superset.views.datasource.utils import get_samples +from superset.views.error_handling import handle_api_exception from superset.views.utils import sanitize_datasource_data diff --git a/superset/views/error_handling.py b/superset/views/error_handling.py new file mode 100644 index 000000000..b9d0a4108 --- /dev/null +++ b/superset/views/error_handling.py @@ -0,0 +1,222 @@ +# 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 __future__ import annotations + +import dataclasses +import functools +import logging +import typing +from importlib.resources import files +from typing import Any, Callable, cast + +from flask import ( + Flask, + redirect, + request, + Response, + send_file, +) +from flask_wtf.csrf import CSRFError +from sqlalchemy import exc +from werkzeug.exceptions import HTTPException + +from superset import appbuilder +from superset.commands.exceptions import CommandException, CommandInvalidError +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import ( + SupersetErrorException, + SupersetErrorsException, + SupersetException, + SupersetSecurityException, +) +from superset.superset_typing import FlaskResponse +from superset.utils import core as utils, json + +if typing.TYPE_CHECKING: + from superset.views.base import BaseSupersetView + + +logger = logging.getLogger(__name__) + +JSON_MIMETYPE = "application/json; charset=utf-8" + + +def get_error_level_from_status( + status_code: int, +) -> ErrorLevel: + if status_code < 400: + return ErrorLevel.INFO + if status_code < 500: + return ErrorLevel.WARNING + return ErrorLevel.ERROR + + +def json_error_response( + error_details: str | SupersetError | list[SupersetError] | None = None, + status: int = 500, + payload: dict[str, Any] | None = None, +) -> FlaskResponse: + payload = payload or {} + + if isinstance(error_details, list): + payload["errors"] = [dataclasses.asdict(error) for error in error_details] + elif isinstance(error_details, SupersetError): + payload["errors"] = [dataclasses.asdict(error_details)] + elif isinstance(error_details, str): + payload["error"] = error_details + + return Response( + json.dumps(payload, default=json.json_iso_dttm_ser, ignore_nan=True), + status=status, + mimetype=JSON_MIMETYPE, + ) + + +def handle_api_exception( + f: Callable[..., FlaskResponse], +) -> Callable[..., FlaskResponse]: + """ + A decorator to catch superset exceptions. Use it after the @api decorator above + so superset exception handler is triggered before the handler for generic + exceptions. + """ + + def wraps(self: BaseSupersetView, *args: Any, **kwargs: Any) -> FlaskResponse: + try: + return f(self, *args, **kwargs) + except SupersetSecurityException as ex: + logger.warning("SupersetSecurityException", exc_info=True) + return json_error_response([ex.error], status=ex.status, payload=ex.payload) + except SupersetErrorsException as ex: + logger.warning(ex, exc_info=True) + return json_error_response(ex.errors, status=ex.status) + except SupersetErrorException as ex: + 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) + return json_error_response( + utils.error_msg_from_exception(ex), status=ex.status + ) + except HTTPException as ex: + logger.exception(ex) + return json_error_response( + utils.error_msg_from_exception(ex), status=cast(int, ex.code) + ) + except (exc.IntegrityError, exc.DatabaseError, exc.DataError) as ex: + logger.exception(ex) + return json_error_response(utils.error_msg_from_exception(ex), status=422) + except Exception as ex: # pylint: disable=broad-except + logger.exception(ex) + return json_error_response(utils.error_msg_from_exception(ex)) + + return functools.update_wrapper(wraps, f) + + +def set_app_error_handlers(app: Flask) -> None: + """ + Set up error handlers for the Flask app + Refer to SIP-40 and SIP-41 for more details on the error handling strategy + """ + + @app.errorhandler(SupersetErrorException) + def show_superset_error(ex: SupersetErrorException) -> FlaskResponse: + logger.warning("SupersetErrorException", exc_info=True) + return json_error_response([ex.error], status=ex.status) + + @app.errorhandler(SupersetErrorsException) + def show_superset_errors(ex: SupersetErrorsException) -> FlaskResponse: + logger.warning("SupersetErrorsException", exc_info=True) + return json_error_response(ex.errors, status=ex.status) + + @app.errorhandler(CSRFError) + def refresh_csrf_token(ex: CSRFError) -> FlaskResponse: + """Redirect to login if the CSRF token is expired""" + logger.warning("Refresh CSRF token error", exc_info=True) + + if request.is_json: + return show_http_exception(ex) + + return redirect(appbuilder.get_url_for_login) + + @app.errorhandler(HTTPException) + def show_http_exception(ex: HTTPException) -> FlaskResponse: + logger.warning("HTTPException", exc_info=True) + if ( + "text/html" in request.accept_mimetypes + and not app.config["DEBUG"] + and ex.code in {404, 500} + ): + path = files("superset") / f"static/assets/{ex.code}.html" + return send_file(path, max_age=0), ex.code + + return json_error_response( + [ + SupersetError( + message=utils.error_msg_from_exception(ex), + error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, + level=ErrorLevel.ERROR, + ), + ], + status=ex.code or 500, + ) + + @app.errorhandler(CommandException) + def show_command_errors(ex: CommandException) -> FlaskResponse: + """ + 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 + """ + logger.warning("CommandException", exc_info=True) + if "text/html" in request.accept_mimetypes and not app.config["DEBUG"]: + path = files("superset") / "static/assets/500.html" + return send_file(path, max_age=0), 500 + + extra = ex.normalized_messages() if isinstance(ex, CommandInvalidError) else {} + return json_error_response( + [ + SupersetError( + message=ex.message, + error_type=SupersetErrorType.GENERIC_COMMAND_ERROR, + level=get_error_level_from_status(ex.status), + extra=extra, + ), + ], + status=ex.status, + ) + + @app.errorhandler(Exception) + @app.errorhandler(500) + def show_unexpected_exception(ex: Exception) -> FlaskResponse: + """Catch-all, to ensure all errors from the backend conform to SIP-40""" + logger.warning("Exception", exc_info=True) + logger.exception(ex) + if "text/html" in request.accept_mimetypes and not app.config["DEBUG"]: + path = files("superset") / "static/assets/500.html" + return send_file(path, max_age=0), 500 + + return json_error_response( + [ + SupersetError( + message=utils.error_msg_from_exception(ex), + error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, + level=ErrorLevel.ERROR, + ), + ], + )