Skip to content

Commit

Permalink
Make stacktraces available in many more cases (#6299)
Browse files Browse the repository at this point in the history
* Wrap <LoadableRenderer /> with <ErrorBoundary />

It appears that since the introduction of <SuperChart />, errors in the
visualization javascript (which are somewhat common and expected,
especially as we'll support plugins) were not handled and the whole
page would throw and go missing.

Here I'm introducing a new <ErrorBoundary /> component that elegantly
wraps other
components and handles errors. It's inspired by:
https://reactjs.org/docs/error-boundaries.html

The default behavior of the component is to simply surface the error
as an <Alert bsStyle="danger" /> and exposes the React stacktrace
when clicking on the error.

It's also possible to use component and pass an onError handler and not
show the default message.

This also fixes some minor bugs in TimeTable.

* Addressing comments

* Getting more stack traces

* Adressing comments
  • Loading branch information
mistercrunch authored Nov 8, 2018
1 parent 1fcfda4 commit 4934605
Show file tree
Hide file tree
Showing 9 changed files with 37 additions and 25 deletions.
10 changes: 6 additions & 4 deletions superset/assets/src/chart/Chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ const propTypes = {
// state
chartAlert: PropTypes.string,
chartStatus: PropTypes.string,
chartStackTrace: PropTypes.string,
queryResponse: PropTypes.object,
triggerQuery: PropTypes.bool,
refreshOverlayVisible: PropTypes.bool,
Expand Down Expand Up @@ -90,10 +91,10 @@ class Chart extends React.PureComponent {
});
}

handleRenderFailure(e) {
handleRenderFailure(error, info) {
const { actions, chartId } = this.props;
console.warn(e); // eslint-disable-line
actions.chartRenderingFailed(e.toString(), chartId);
console.warn(error); // eslint-disable-line
actions.chartRenderingFailed(error.toString(), chartId, info ? info.componentStack : null);
}

prepareChartProps() {
Expand Down Expand Up @@ -153,6 +154,7 @@ class Chart extends React.PureComponent {
width,
height,
chartAlert,
chartStackTrace,
chartStatus,
errorMessage,
onDismissRefreshOverlay,
Expand Down Expand Up @@ -183,7 +185,7 @@ class Chart extends React.PureComponent {
<StackTraceMessage
message={chartAlert}
link={queryResponse ? queryResponse.link : null}
stackTrace={queryResponse ? queryResponse.stacktrace : null}
stackTrace={chartStackTrace}
/>
)}

Expand Down
4 changes: 2 additions & 2 deletions superset/assets/src/chart/chartAction.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ export function chartUpdateFailed(queryResponse, key) {
}

export const CHART_RENDERING_FAILED = 'CHART_RENDERING_FAILED';
export function chartRenderingFailed(error, key) {
return { type: CHART_RENDERING_FAILED, error, key };
export function chartRenderingFailed(error, key, stackTrace) {
return { type: CHART_RENDERING_FAILED, error, key, stackTrace };
}

export const CHART_RENDERING_SUCCEEDED = 'CHART_RENDERING_SUCCEEDED';
Expand Down
4 changes: 4 additions & 0 deletions superset/assets/src/chart/chartReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export const chart = {
id: 0,
chartAlert: null,
chartStatus: 'loading',
chartStackTrace: null,
chartUpdateEndTime: null,
chartUpdateStartTime: 0,
latestQueryFormData: {},
Expand Down Expand Up @@ -35,6 +36,7 @@ export default function chartReducer(charts = {}, action) {
return {
...state,
chartStatus: 'loading',
chartStackTrace: null,
chartAlert: null,
chartUpdateEndTime: null,
chartUpdateStartTime: now(),
Expand All @@ -56,6 +58,7 @@ export default function chartReducer(charts = {}, action) {
[actions.CHART_RENDERING_FAILED](state) {
return { ...state,
chartStatus: 'failed',
chartStackTrace: action.stackTrace,
chartAlert: t('An error occurred while rendering the visualization: %s', action.error),
};
},
Expand All @@ -78,6 +81,7 @@ export default function chartReducer(charts = {}, action) {
chartAlert: action.queryResponse ? action.queryResponse.error : t('Network error.'),
chartUpdateEndTime: now(),
queryResponse: action.queryResponse,
chartStackTrace: action.queryResponse ? action.queryResponse.stacktrace : null,
};
},
[actions.TRIGGER_QUERY](state) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ class ExploreChartPanel extends React.PureComponent {
height={parseInt(this.props.height, 10) - headerHeight}
annotationData={chart.annotationData}
chartAlert={chart.chartAlert}
chartStackTrace={chart.chartStackTrace}
chartId={chart.id}
chartStatus={chart.chartStatus}
datasource={this.props.datasource}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
export default function transformProps(chartProps) {
const { height, datasource, formData, payload } = chartProps;
const {
columnCollection = 0,
columnCollection = [],
groupby,
metrics,
url,
Expand Down
4 changes: 2 additions & 2 deletions superset/views/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@
from superset import appbuilder, security_manager
from superset.common.query_context import QueryContext
from superset.models.core import Log
from .base import api, BaseSupersetView, data_payload_response, handle_superset_exception
from .base import api, BaseSupersetView, data_payload_response, handle_api_exception


class Api(BaseSupersetView):
@Log.log_this
@api
@handle_superset_exception
@handle_api_exception
@has_access_api
@expose('/v1/query/', methods=['POST'])
def query(self):
Expand Down
26 changes: 16 additions & 10 deletions superset/views/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,23 +86,29 @@ def wraps(self, *args, **kwargs):
return functools.update_wrapper(wraps, f)


def handle_superset_exception(f):
def handle_api_exception(f):
"""
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, *args, **kwargs):
try:
return f(self, *args, **kwargs)
except SupersetSecurityException as sse:
logging.exception(sse)
return json_error_response(utils.error_msg_from_exception(sse),
status=sse.status,
link=sse.link)
except SupersetException as se:
logging.exception(se)
return json_error_response(utils.error_msg_from_exception(se),
status=se.status)
except SupersetSecurityException as e:
logging.exception(e)
return json_error_response(utils.error_msg_from_exception(e),
status=e.status,
stacktrace=traceback.format_exc(),
link=e.link)
except SupersetException as e:
logging.exception(e)
return json_error_response(utils.error_msg_from_exception(e),
stacktrace=traceback.format_exc(),
status=e.status)
except Exception as e:
logging.exception(e)
return json_error_response(utils.error_msg_from_exception(e),
stacktrace=traceback.format_exc())
return functools.update_wrapper(wraps, f)


Expand Down
10 changes: 5 additions & 5 deletions superset/views/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
api, BaseSupersetView,
check_ownership,
CsvResponse, data_payload_response, DeleteMixin, generate_download_headers,
get_error_msg, handle_superset_exception, json_error_response, json_success,
get_error_msg, handle_api_exception, json_error_response, json_success,
SupersetFilter, SupersetModelView, YamlExportMixin,
)
from .utils import bootstrap_user_data
Expand Down Expand Up @@ -1108,7 +1108,6 @@ def get_samples(self, viz_obj):
'data': viz_obj.get_samples(),
})

@handle_superset_exception
def generate_json(
self, datasource_type, datasource_id, form_data,
csv=False, query=False, force=False, results=False,
Expand Down Expand Up @@ -1176,6 +1175,7 @@ def annotation_json(self, layer_id):
@log_this
@api
@has_access_api
@handle_api_exception
@expose('/explore_json/<datasource_type>/<datasource_id>/', methods=['GET', 'POST'])
@expose('/explore_json/', methods=['GET', 'POST'])
def explore_json(self, datasource_type=None, datasource_id=None):
Expand Down Expand Up @@ -1353,7 +1353,7 @@ def explore(self, datasource_type=None, datasource_id=None):
standalone_mode=standalone)

@api
@handle_superset_exception
@handle_api_exception
@has_access_api
@expose('/filter/<datasource_type>/<datasource_id>/<column>/')
def filter(self, datasource_type, datasource_id, column):
Expand Down Expand Up @@ -2609,7 +2609,7 @@ def csv(self, client_id):
return response

@api
@handle_superset_exception
@handle_api_exception
@has_access
@expose('/fetch_datasource_metadata')
@log_this
Expand Down Expand Up @@ -2800,7 +2800,7 @@ def sqllab(self):
)

@api
@handle_superset_exception
@handle_api_exception
@has_access_api
@expose('/slice_query/<slice_id>/')
def slice_query(self, slice_id):
Expand Down
1 change: 0 additions & 1 deletion superset/viz.py
Original file line number Diff line number Diff line change
Expand Up @@ -445,7 +445,6 @@ def get_df_payload(self, query_obj=None, **kwargs):
logging.warning('Could not cache key {}'.format(cache_key))
logging.exception(e)
cache.delete(cache_key)

return {
'cache_key': self._any_cache_key,
'cached_dttm': self._any_cached_dttm,
Expand Down

0 comments on commit 4934605

Please sign in to comment.