From 9c3a169196aaa88af16599c9a8988674718b49d6 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Fri, 30 Jul 2021 15:18:38 -0700 Subject: [PATCH 1/3] fix(dashboard): FilterBox JS error when datasets API is slow --- .pylintrc | 2 +- superset-frontend/src/chart/Chart.jsx | 28 +++++++++++++-- superset-frontend/src/chart/ChartRenderer.jsx | 10 +++--- superset-frontend/src/chart/chartReducer.ts | 19 ++++++++++ .../components/gridComponents/Chart.jsx | 3 +- superset-frontend/src/dashboard/constants.ts | 35 +++++++++++++++++++ .../src/dashboard/containers/Chart.jsx | 4 ++- .../{transformProps.js => transformProps.ts} | 8 +++-- .../src/visualizations/FilterBox/types.ts | 29 +++++++++++++++ superset/connectors/base/models.py | 7 +++- 10 files changed, 131 insertions(+), 14 deletions(-) create mode 100644 superset-frontend/src/dashboard/constants.ts rename superset-frontend/src/visualizations/FilterBox/{transformProps.js => transformProps.ts} (91%) create mode 100644 superset-frontend/src/visualizations/FilterBox/types.ts diff --git a/.pylintrc b/.pylintrc index e94c7becd4b38..837291c5b6da0 100644 --- a/.pylintrc +++ b/.pylintrc @@ -373,7 +373,7 @@ max-locals=15 max-returns=10 # Maximum number of branch for function / method body -max-branches=12 +max-branches=15 # Maximum number of statements in function / method body max-statements=50 diff --git a/superset-frontend/src/chart/Chart.jsx b/superset-frontend/src/chart/Chart.jsx index d056fe9dbe176..d66dd86b15e05 100644 --- a/superset-frontend/src/chart/Chart.jsx +++ b/superset-frontend/src/chart/Chart.jsx @@ -22,8 +22,9 @@ import Alert from 'src/components/Alert'; import { styled, logging, t } from '@superset-ui/core'; import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags'; +import { PLACEHOLDER_DATASOURCE } from 'src/dashboard/constants'; import Button from 'src/components/Button'; -import Loading from '../components/Loading'; +import Loading from 'src/components/Loading'; import ErrorBoundary from '../components/ErrorBoundary'; import ChartRenderer from './ChartRenderer'; import { ChartErrorMessage } from './ChartErrorMessage'; @@ -164,10 +165,32 @@ class Chart extends React.PureComponent { } renderErrorMessage(queryResponse) { - const { chartId, chartAlert, chartStackTrace, dashboardId } = this.props; + const { + chartId, + chartAlert, + chartStackTrace, + datasource, + dashboardId, + height, + } = this.props; const error = queryResponse?.errors?.[0]; const message = chartAlert || queryResponse?.message; + + // if datasource is still loading, don't render JS errors + if (chartAlert && datasource === PLACEHOLDER_DATASOURCE) { + return ( + + + + ); + } + return ( this.renderErrorMessage(item)); } + if (errorMessage) { return ( [ + chartId, + // if render has failed, clear error message, + // which will trigger a re-render + chart.chartStatus === 'failed' + ? { + ...chart, + chartStatus: '', + chartStackTrace: null, + chartAlert: null, + } + : chart, + ]), + ); + } + if (action.type in actionHandlers) { return { ...charts, diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx index cbeaa66fe27c2..ae40888fcc4dc 100644 --- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx +++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx @@ -20,6 +20,7 @@ import cx from 'classnames'; import React from 'react'; import PropTypes from 'prop-types'; import { styled } from '@superset-ui/core'; +import { isEqual } from 'lodash'; import { exportChart, getExploreLongUrl } from 'src/explore/exploreUtils'; import ChartContainer from 'src/chart/ChartContainer'; @@ -126,7 +127,7 @@ export default class Chart extends React.Component { nextState.width !== this.state.width || nextState.height !== this.state.height || nextState.descriptionHeight !== this.state.descriptionHeight || - nextProps.datasource !== this.props.datasource + !isEqual(nextProps.datasource, this.props.datasource) ) { return true; } diff --git a/superset-frontend/src/dashboard/constants.ts b/superset-frontend/src/dashboard/constants.ts new file mode 100644 index 0000000000000..ab3fa29a0b773 --- /dev/null +++ b/superset-frontend/src/dashboard/constants.ts @@ -0,0 +1,35 @@ +/* eslint-disable import/prefer-default-export */ +import { DatasourceType } from '@superset-ui/core'; +import { Datasource } from 'src/dashboard/types'; +/** + * 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. + */ +export const PLACEHOLDER_DATASOURCE: Datasource = { + id: 0, + type: DatasourceType.Table, + uid: '_placeholder_', + datasource_name: '', + table_name: '', + columns: [], + column_types: [], + metrics: [], + column_format: {}, + verbose_map: {}, + main_dttm_col: '', + description: '', +}; diff --git a/superset-frontend/src/dashboard/containers/Chart.jsx b/superset-frontend/src/dashboard/containers/Chart.jsx index 4bb7f35036cea..4fdfcfdb952cd 100644 --- a/superset-frontend/src/dashboard/containers/Chart.jsx +++ b/superset-frontend/src/dashboard/containers/Chart.jsx @@ -35,6 +35,7 @@ import { } from '../util/activeDashboardFilters'; import getFormDataWithExtraFilters from '../util/charts/getFormDataWithExtraFilters'; import Chart from '../components/gridComponents/Chart'; +import { PLACEHOLDER_DATASOURCE } from '../constants'; const EMPTY_FILTERS = {}; @@ -55,7 +56,8 @@ function mapStateToProps( const { id } = ownProps; const chart = chartQueries[id] || {}; const datasource = - (chart && chart.form_data && datasources[chart.form_data.datasource]) || {}; + (chart && chart.form_data && datasources[chart.form_data.datasource]) || + PLACEHOLDER_DATASOURCE; const { colorScheme, colorNamespace } = dashboardState; // note: this method caches filters if possible to prevent render cascades diff --git a/superset-frontend/src/visualizations/FilterBox/transformProps.js b/superset-frontend/src/visualizations/FilterBox/transformProps.ts similarity index 91% rename from superset-frontend/src/visualizations/FilterBox/transformProps.js rename to superset-frontend/src/visualizations/FilterBox/transformProps.ts index dda5558c6c09d..19a962ddd1dcd 100644 --- a/superset-frontend/src/visualizations/FilterBox/transformProps.js +++ b/superset-frontend/src/visualizations/FilterBox/transformProps.ts @@ -16,16 +16,18 @@ * specific language governing permissions and limitations * under the License. */ +import { FilterBoxChartProps } from './types'; + const NOOP = () => {}; -export default function transformProps(chartProps) { +export default function transformProps(chartProps: FilterBoxChartProps) { const { datasource, formData, hooks, initialValues, queriesData, - rawDatasource, + rawDatasource = {}, rawFormData, width, height, @@ -44,7 +46,7 @@ export default function transformProps(chartProps) { showSqlaTimeColumn, showSqlaTimeGranularity, } = formData; - const { verboseMap } = datasource; + const { verboseMap = {} } = datasource; const filterConfigs = formData.filterConfigs || []; const filtersFields = filterConfigs.map(flt => ({ diff --git a/superset-frontend/src/visualizations/FilterBox/types.ts b/superset-frontend/src/visualizations/FilterBox/types.ts new file mode 100644 index 0000000000000..316a29f9bbef6 --- /dev/null +++ b/superset-frontend/src/visualizations/FilterBox/types.ts @@ -0,0 +1,29 @@ +/** + * 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. + */ +import { ChartProps, Datasource } from '@superset-ui/core'; + +export interface FilterConfig { + column: string; + label: string; +} + +export type FilterBoxChartProps = ChartProps & { + datasource?: Datasource; + formData: ChartProps['formData'] & { filterConfigs: FilterConfig[] }; +}; diff --git a/superset/connectors/base/models.py b/superset/connectors/base/models.py index d56f8b85f00d2..3813bf73dc63c 100644 --- a/superset/connectors/base/models.py +++ b/superset/connectors/base/models.py @@ -293,11 +293,16 @@ def data_for_slices(self, slices: List[Slice]) -> Dict[str, Any]: (metric.get("column") or {}).get("column_name") ) - # pull out all required columns from the form_data + # Columns used in filters for filter_ in form_data.get("adhoc_filters") or []: if filter_["clause"] == "WHERE" and filter_.get("subject"): column_names.add(filter_.get("subject")) + # columns used by Filter Box + for filter_config in form_data.get("filter_configs") or []: + if filter_config and "column" in filter_config: + column_names.add(filter_config["column"]) + for param in COLUMN_FORM_DATA_PARAMS: for column in utils.get_iterable(form_data.get(param) or []): column_names.add(column) From 99085fa564dffb1e08d75f2322a2fe83aa63dd87 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Fri, 30 Jul 2021 17:24:44 -0700 Subject: [PATCH 2/3] Pythonic refactor --- setup.cfg | 2 +- superset/connectors/base/models.py | 27 ++++++++++++++++----------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/setup.cfg b/setup.cfg index 1e16680ee0b4e..c5eeeef35bfdb 100644 --- a/setup.cfg +++ b/setup.cfg @@ -30,7 +30,7 @@ combine_as_imports = true include_trailing_comma = true line_length = 88 known_first_party = superset -known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,graphlib,holidays,humanize,isodate,jinja2,jwt,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pgsanity,pkg_resources,polyline,prison,progress,pyarrow,pyhive,pyparsing,pytest,pytest_mock,pytz,redis,requests,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,tabulate,typing_extensions,werkzeug,wtforms,wtforms_json,yaml +known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,holidays,humanize,isodate,jinja2,jwt,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pgsanity,pkg_resources,polyline,prison,progress,pyarrow,pyhive,pyparsing,pytest,pytest_mock,pytz,redis,requests,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,tabulate,typing_extensions,werkzeug,wtforms,wtforms_json,yaml multi_line_output = 3 order_by_type = false diff --git a/superset/connectors/base/models.py b/superset/connectors/base/models.py index 3813bf73dc63c..88c4f4098f89b 100644 --- a/superset/connectors/base/models.py +++ b/superset/connectors/base/models.py @@ -287,25 +287,30 @@ def data_for_slices(self, slices: List[Slice]) -> Dict[str, Any]: for param in METRIC_FORM_DATA_PARAMS: for metric in utils.get_iterable(form_data.get(param) or []): metric_names.add(utils.get_metric_name(metric)) - if utils.is_adhoc_metric(metric): column_names.add( (metric.get("column") or {}).get("column_name") ) - # Columns used in filters - for filter_ in form_data.get("adhoc_filters") or []: - if filter_["clause"] == "WHERE" and filter_.get("subject"): - column_names.add(filter_.get("subject")) + # Columns used in query filters + column_names.update( + filter_["subject"] + for filter_ in form_data.get("adhoc_filters") or [] + if filter_.get("clause") == "WHERE" and filter_.get("subject") + ) # columns used by Filter Box - for filter_config in form_data.get("filter_configs") or []: - if filter_config and "column" in filter_config: - column_names.add(filter_config["column"]) + column_names.update( + filter_config["column"] + for filter_config in form_data.get("filter_configs") or [] + if "column" in filter_config + ) - for param in COLUMN_FORM_DATA_PARAMS: - for column in utils.get_iterable(form_data.get(param) or []): - column_names.add(column) + column_names.update( + column + for column in utils.get_iterable(form_data.get(param) or []) + for param in COLUMN_FORM_DATA_PARAMS + ) filtered_metrics = [ metric From 32e6cc891b150e44caa1a77f0582b583f9715c95 Mon Sep 17 00:00:00 2001 From: serenajiang Date: Fri, 30 Jul 2021 18:21:59 -0700 Subject: [PATCH 3/3] Update setup.cfg --- setup.cfg | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.cfg b/setup.cfg index c5eeeef35bfdb..1e16680ee0b4e 100644 --- a/setup.cfg +++ b/setup.cfg @@ -30,7 +30,7 @@ combine_as_imports = true include_trailing_comma = true line_length = 88 known_first_party = superset -known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,holidays,humanize,isodate,jinja2,jwt,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pgsanity,pkg_resources,polyline,prison,progress,pyarrow,pyhive,pyparsing,pytest,pytest_mock,pytz,redis,requests,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,tabulate,typing_extensions,werkzeug,wtforms,wtforms_json,yaml +known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,graphlib,holidays,humanize,isodate,jinja2,jwt,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pgsanity,pkg_resources,polyline,prison,progress,pyarrow,pyhive,pyparsing,pytest,pytest_mock,pytz,redis,requests,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,tabulate,typing_extensions,werkzeug,wtforms,wtforms_json,yaml multi_line_output = 3 order_by_type = false