Skip to content

Commit

Permalink
fix: improve unusual error handling for api/chart/data
Browse files Browse the repository at this point in the history
I've recently encountered some errors on the dashboard page, within Chart widgets where html is displayed as a result of calling the `api/chart/data` endpoint, and presumably while hitting some 502 / 504s-type error. The error is hard to reproduce, but I'm thinking the approach bellow should improve / prevent that endpoint from ever returning html.

Two changes here:
- sprinkling the @handle_api_error decorator in critical related views
- improving the 500 flask error handler to be a bit smarter, hoping it'll catch some of the problems and return json when expecting it
  • Loading branch information
mistercrunch committed Jun 21, 2024
1 parent 514eda8 commit d0d1bf0
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 10 deletions.
11 changes: 10 additions & 1 deletion superset/charts/data/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,12 @@
get_user_id,
)
from superset.utils.decorators import logs_context
from superset.views.base import CsvResponse, generate_download_headers, XlsxResponse
from superset.views.base import (
CsvResponse,
generate_download_headers,
handle_api_exception,
XlsxResponse,
)
from superset.views.base_api import statsd_metrics

if TYPE_CHECKING:
Expand All @@ -71,6 +76,7 @@ class ChartDataRestApi(ChartRestApi):
action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.data",
log_to_statsd=False,
)
@handle_api_exception
def get_data(self, pk: int) -> Response:
"""
Take a chart ID and uses the query context stored when the chart was saved
Expand Down Expand Up @@ -184,6 +190,7 @@ def get_data(self, pk: int) -> Response:
action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.data",
log_to_statsd=False,
)
@handle_api_exception
def data(self) -> Response:
"""
Take a query context constructed in the client and return payload
Expand Down Expand Up @@ -224,6 +231,7 @@ def data(self) -> Response:
$ref: '#/components/responses/500'
"""
json_body = None

if request.is_json:
json_body = request.json
elif request.form.get("form_data"):
Expand Down Expand Up @@ -269,6 +277,7 @@ def data(self) -> Response:
f".data_from_cache",
log_to_statsd=False,
)
@handle_api_exception
def data_from_cache(self, cache_key: str) -> Response:
"""
Take a query context cache key and return payload
Expand Down
1 change: 1 addition & 0 deletions superset/views/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,7 @@ def show_command_errors(ex: CommandException) -> FlaskResponse:

# Catch-all, to ensure all errors from the backend conform to SIP-40
@superset_app.errorhandler(Exception)
@superset_app.errorhandler(500)
def show_unexpected_exception(ex: Exception) -> FlaskResponse:
logger.exception(ex)
if "text/html" in request.accept_mimetypes and not config["DEBUG"]:
Expand Down
10 changes: 1 addition & 9 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
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,
Expand Down Expand Up @@ -85,7 +85,6 @@
data_payload_response,
deprecated,
generate_download_headers,
get_error_msg,
handle_api_exception,
json_error_response,
json_success,
Expand Down Expand Up @@ -886,13 +885,6 @@ def fetch_datasource_metadata(self) -> FlaskResponse:
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:
Expand Down

0 comments on commit d0d1bf0

Please sign in to comment.