From b73d7baedfb000dd39816f8194c1ad6f4d4c627d Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Fri, 30 Jul 2021 22:06:29 -0700 Subject: [PATCH] fix(dashboard): FilterBox JS error when datasets API is slow (#15993) --- .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 | 26 +++++++++----- 10 files changed, 143 insertions(+), 21 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..88c4f4098f89b 100644 --- a/superset/connectors/base/models.py +++ b/superset/connectors/base/models.py @@ -287,20 +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") ) - # pull out all required columns from the form_data - 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") + ) - for param in COLUMN_FORM_DATA_PARAMS: - for column in utils.get_iterable(form_data.get(param) or []): - column_names.add(column) + # columns used by Filter Box + column_names.update( + filter_config["column"] + for filter_config in form_data.get("filter_configs") or [] + if "column" in filter_config + ) + + 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