superset-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From maximebeauche...@apache.org
Subject [incubator-superset] branch master updated: Make stacktraces available in many more cases (#6299)
Date Thu, 08 Nov 2018 17:38:17 GMT
This is an automated email from the ASF dual-hosted git repository.

maximebeauchemin pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-superset.git


The following commit(s) were added to refs/heads/master by this push:
     new 4934605  Make stacktraces available in many more cases (#6299)
4934605 is described below

commit 49346050439d66179e1de19c0bffa4ced3a8dbe6
Author: Maxime Beauchemin <maximebeauchemin@gmail.com>
AuthorDate: Thu Nov 8 09:38:10 2018 -0800

    Make stacktraces available in many more cases (#6299)
    
    * 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
---
 superset/assets/src/chart/Chart.jsx                | 10 +++++----
 superset/assets/src/chart/chartAction.js           |  4 ++--
 superset/assets/src/chart/chartReducer.js          |  4 ++++
 .../src/explore/components/ExploreChartPanel.jsx   |  1 +
 .../src/visualizations/TimeTable/transformProps.js |  2 +-
 superset/views/api.py                              |  4 ++--
 superset/views/base.py                             | 26 +++++++++++++---------
 superset/views/core.py                             | 10 ++++-----
 superset/viz.py                                    |  1 -
 9 files changed, 37 insertions(+), 25 deletions(-)

diff --git a/superset/assets/src/chart/Chart.jsx b/superset/assets/src/chart/Chart.jsx
index d0d3461..d2abb6b 100644
--- a/superset/assets/src/chart/Chart.jsx
+++ b/superset/assets/src/chart/Chart.jsx
@@ -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,
@@ -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() {
@@ -153,6 +154,7 @@ class Chart extends React.PureComponent {
       width,
       height,
       chartAlert,
+      chartStackTrace,
       chartStatus,
       errorMessage,
       onDismissRefreshOverlay,
@@ -183,7 +185,7 @@ class Chart extends React.PureComponent {
           <StackTraceMessage
             message={chartAlert}
             link={queryResponse ? queryResponse.link : null}
-            stackTrace={queryResponse ? queryResponse.stacktrace : null}
+            stackTrace={chartStackTrace}
           />
         )}
 
diff --git a/superset/assets/src/chart/chartAction.js b/superset/assets/src/chart/chartAction.js
index fb4ffd6..b816d87 100644
--- a/superset/assets/src/chart/chartAction.js
+++ b/superset/assets/src/chart/chartAction.js
@@ -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';
diff --git a/superset/assets/src/chart/chartReducer.js b/superset/assets/src/chart/chartReducer.js
index f6b608c..3936f9c 100644
--- a/superset/assets/src/chart/chartReducer.js
+++ b/superset/assets/src/chart/chartReducer.js
@@ -7,6 +7,7 @@ export const chart = {
   id: 0,
   chartAlert: null,
   chartStatus: 'loading',
+  chartStackTrace: null,
   chartUpdateEndTime: null,
   chartUpdateStartTime: 0,
   latestQueryFormData: {},
@@ -35,6 +36,7 @@ export default function chartReducer(charts = {}, action) {
       return {
         ...state,
         chartStatus: 'loading',
+        chartStackTrace: null,
         chartAlert: null,
         chartUpdateEndTime: null,
         chartUpdateStartTime: now(),
@@ -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),
       };
     },
@@ -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) {
diff --git a/superset/assets/src/explore/components/ExploreChartPanel.jsx b/superset/assets/src/explore/components/ExploreChartPanel.jsx
index efff4a0..1cdc94c 100644
--- a/superset/assets/src/explore/components/ExploreChartPanel.jsx
+++ b/superset/assets/src/explore/components/ExploreChartPanel.jsx
@@ -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}
diff --git a/superset/assets/src/visualizations/TimeTable/transformProps.js b/superset/assets/src/visualizations/TimeTable/transformProps.js
index d52088b..8492ce2 100644
--- a/superset/assets/src/visualizations/TimeTable/transformProps.js
+++ b/superset/assets/src/visualizations/TimeTable/transformProps.js
@@ -1,7 +1,7 @@
 export default function transformProps(chartProps) {
   const { height, datasource, formData, payload } = chartProps;
   const {
-    columnCollection = 0,
+    columnCollection = [],
     groupby,
     metrics,
     url,
diff --git a/superset/views/api.py b/superset/views/api.py
index c1d2714..5f684f3 100644
--- a/superset/views/api.py
+++ b/superset/views/api.py
@@ -8,13 +8,13 @@ from flask_appbuilder.security.decorators import has_access_api
 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):
diff --git a/superset/views/base.py b/superset/views/base.py
index 657242f..31ab1ce 100644
--- a/superset/views/base.py
+++ b/superset/views/base.py
@@ -86,7 +86,7 @@ def api(f):
     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.
@@ -94,15 +94,21 @@ def handle_superset_exception(f):
     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)
 
 
diff --git a/superset/views/core.py b/superset/views/core.py
index ef4c4ec..9b79b75 100755
--- a/superset/views/core.py
+++ b/superset/views/core.py
@@ -46,7 +46,7 @@ from .base import (
     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
@@ -1108,7 +1108,6 @@ class Superset(BaseSupersetView):
             '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,
@@ -1176,6 +1175,7 @@ class Superset(BaseSupersetView):
     @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):
@@ -1353,7 +1353,7 @@ class Superset(BaseSupersetView):
             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):
@@ -2609,7 +2609,7 @@ class Superset(BaseSupersetView):
         return response
 
     @api
-    @handle_superset_exception
+    @handle_api_exception
     @has_access
     @expose('/fetch_datasource_metadata')
     @log_this
@@ -2800,7 +2800,7 @@ class Superset(BaseSupersetView):
         )
 
     @api
-    @handle_superset_exception
+    @handle_api_exception
     @has_access_api
     @expose('/slice_query/<slice_id>/')
     def slice_query(self, slice_id):
diff --git a/superset/viz.py b/superset/viz.py
index fba68ae..91b48f2 100644
--- a/superset/viz.py
+++ b/superset/viz.py
@@ -445,7 +445,6 @@ class BaseViz(object):
                     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,


Mime
View raw message