Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dashboard): FilterBox JS error when datasets API is slow #15993

Merged
merged 3 commits into from
Jul 31, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,7 @@ max-locals=15
max-returns=10

# Maximum number of branch for function / method body
max-branches=12
max-branches=15
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow more if branches instead of suppressing the too-many-branches error inline.

cc @villebro @john-bodley @etr2460

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! This is an anti-pattern in my opinion. The check is there for a reason to prevent non-readable code. If one must override this, it should be done locally and not globally.


# Maximum number of statements in function / method body
max-statements=50
Expand Down
28 changes: 26 additions & 2 deletions superset-frontend/src/chart/Chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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 (
<Styles
data-ui-anchor="chart"
className="chart-container"
data-test="chart-container"
height={height}
>
<Loading />
</Styles>
);
}

return (
<ChartErrorMessage
chartId={chartId}
Expand Down Expand Up @@ -198,6 +221,7 @@ class Chart extends React.PureComponent {
if (chartStatus === 'failed') {
return queriesResponse.map(item => this.renderErrorMessage(item));
}

if (errorMessage) {
return (
<Alert
Expand Down
10 changes: 5 additions & 5 deletions superset-frontend/src/chart/ChartRenderer.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
* specific language governing permissions and limitations
* under the License.
*/
import { snakeCase } from 'lodash';
import { snakeCase, isEqual } from 'lodash';
import PropTypes from 'prop-types';
import React from 'react';
import { SuperChart, logging, Behavior } from '@superset-ui/core';
import { Logger, LOG_ACTIONS_RENDER_CHART } from '../logger/LogUtils';
import { Logger, LOG_ACTIONS_RENDER_CHART } from 'src/logger/LogUtils';

const propTypes = {
annotationData: PropTypes.object,
Expand Down Expand Up @@ -93,7 +93,7 @@ class ChartRenderer extends React.Component {
nextProps.queriesResponse !== this.props.queriesResponse;
return (
this.hasQueryResponseChange ||
nextProps.datasource !== this.props.datasource ||
!isEqual(nextProps.datasource, this.props.datasource) ||

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

nextProps.annotationData !== this.props.annotationData ||
nextProps.ownState !== this.props.ownState ||
nextProps.filterState !== this.props.filterState ||
Expand Down Expand Up @@ -183,8 +183,8 @@ class ChartRenderer extends React.Component {
const {
width,
height,
annotationData,
datasource,
annotationData,
initialValues,
ownState,
filterState,
Expand Down Expand Up @@ -224,7 +224,7 @@ class ChartRenderer extends React.Component {
width={width}
height={height}
annotationData={annotationData}
datasource={datasource || {}}
datasource={datasource}
initialValues={initialValues}
formData={formData}
ownState={ownState}
Expand Down
19 changes: 19 additions & 0 deletions superset-frontend/src/chart/chartReducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
/* eslint camelcase: 0 */
import { t } from '@superset-ui/core';
import { HYDRATE_DASHBOARD } from 'src/dashboard/actions/hydrate';
import { DatasourcesAction } from 'src/dashboard/actions/datasources';
import { ChartState } from 'src/explore/types';
import { getFormDataFromControls } from 'src/explore/controlUtils';
import { now } from 'src/modules/dates';
Expand Down Expand Up @@ -196,6 +197,24 @@ export default function chartReducer(
if (action.type === HYDRATE_DASHBOARD) {
return { ...action.data.charts };
}
if (action.type === DatasourcesAction.SET_DATASOURCES) {
return Object.fromEntries(
Object.entries(charts).map(([chartId, chart]) => [
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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
}
Expand Down
35 changes: 35 additions & 0 deletions superset-frontend/src/dashboard/constants.ts
Original file line number Diff line number Diff line change
@@ -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: '',
};
4 changes: 3 additions & 1 deletion superset-frontend/src/dashboard/containers/Chart.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {};

Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 => ({
Expand Down
29 changes: 29 additions & 0 deletions superset-frontend/src/visualizations/FilterBox/types.ts
Original file line number Diff line number Diff line change
@@ -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[] };
};
26 changes: 18 additions & 8 deletions superset/connectors/base/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ONE LINER CHALLENGE!

column_names.update(filter_config["column"] for filter_config in form_data.get("filter_configs", []) if filter_config and "column" in filter_config)

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
Expand Down