feat(explore): Don't discard controls with custom sql when changing datasource (#20934)

This commit is contained in:
Kamil Gabryjelski 2022-10-19 15:29:38 +02:00 committed by GitHub
parent ec20c0104e
commit cddc361adc
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
22 changed files with 193 additions and 183 deletions

View File

@ -29,6 +29,7 @@ export interface AdhocColumn {
expressionType: 'SQL';
columnType?: 'BASE_AXIS' | 'SERIES';
timeGrain?: string;
datasourceWarning?: boolean;
}
/**

View File

@ -17,6 +17,6 @@
* under the License.
*/
export default function isDefined(x: unknown) {
export default function isDefined<T>(x: T): x is NonNullable<T> {
return x !== null && x !== undefined;
}

View File

@ -36,6 +36,7 @@ import {
css,
SupersetTheme,
useTheme,
isDefined,
} from '@superset-ui/core';
import {
ControlPanelSectionConfig,
@ -45,6 +46,9 @@ import {
ExpandedControlItem,
sections,
} from '@superset-ui/chart-controls';
import { useSelector } from 'react-redux';
import { rgba } from 'emotion-rgba';
import { kebabCase } from 'lodash';
import Collapse from 'src/components/Collapse';
import Tabs from 'src/components/Tabs';
@ -57,9 +61,6 @@ import { ExploreActions } from 'src/explore/actions/exploreActions';
import { ChartState, ExplorePageState } from 'src/explore/types';
import { Tooltip } from 'src/components/Tooltip';
import Icons from 'src/components/Icons';
import { rgba } from 'emotion-rgba';
import { kebabCase } from 'lodash';
import ControlRow from './ControlRow';
import Control from './Control';
import { ExploreAlert } from './ExploreAlert';
@ -269,6 +270,36 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
const containerRef = useRef<HTMLDivElement>(null);
const controlsTransferred = useSelector<
ExplorePageState,
string[] | undefined
>(state => state.explore.controlsTransferred);
useEffect(() => {
if (props.chart.chartStatus === 'success') {
controlsTransferred?.forEach(controlName => {
const alteredControls = ensureIsArray(
props.controls[controlName].value,
).map(value => {
if (
typeof value === 'object' &&
isDefined(value) &&
'datasourceWarning' in value
) {
return { ...value, datasourceWarning: false };
}
return value;
});
props.actions.setControlValue(controlName, alteredControls);
});
}
}, [
controlsTransferred,
props.actions,
props.chart.chartStatus,
props.controls,
]);
useEffect(() => {
if (
prevDatasource &&
@ -455,7 +486,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
>
<Icons.InfoCircleOutlined
css={css`
${iconStyles}
${iconStyles};
color: ${errorColor};
`}
/>
@ -591,7 +622,7 @@ export const ControlPanelsContainer = (props: ControlPanelsContainerProps) => {
>
<Icons.ExclamationCircleOutlined
css={css`
${iconStyles}
${iconStyles};
color: ${errorColor};
`}
/>

View File

@ -458,6 +458,7 @@ function ExploreViewContainer(props) {
!areObjectsEqual(
props.controls[key].value,
lastQueriedControls[key].value,
{ ignoreFields: ['datasourceWarning'] },
),
);

View File

@ -17,6 +17,7 @@
* under the License.
*/
import React from 'react';
import { t } from '@superset-ui/core';
import { DndItemType } from 'src/explore/components/DndItemType';
import AdhocFilterPopoverTrigger from 'src/explore/components/controls/FilterControl/AdhocFilterPopoverTrigger';
import AdhocFilter from 'src/explore/components/controls/FilterControl/AdhocFilter';
@ -63,6 +64,11 @@ export default function DndAdhocFilterOption({
type={DndItemType.FilterOption}
withCaret
isExtra={adhocFilter.isExtra}
datasourceWarningMessage={
adhocFilter.datasourceWarning
? t('This filter might be incompatible with current dataset')
: undefined
}
/>
</AdhocFilterPopoverTrigger>
);

View File

@ -23,6 +23,8 @@ import {
isFeatureEnabled,
tn,
QueryFormColumn,
t,
isAdhocColumn,
} from '@superset-ui/core';
import {
ColumnMeta,
@ -35,7 +37,6 @@ import OptionWrapper from 'src/explore/components/controls/DndColumnSelectContro
import { OptionSelector } from 'src/explore/components/controls/DndColumnSelectControl/utils';
import { DatasourcePanelDndItem } from 'src/explore/components/DatasourcePanel/types';
import { DndItemType } from 'src/explore/components/DndItemType';
import { useComponentDidUpdate } from 'src/hooks/useComponentDidUpdate';
import ColumnSelectPopoverTrigger from './ColumnSelectPopoverTrigger';
import { DndControlProps } from './types';
import SelectControl from '../SelectControl';
@ -68,34 +69,6 @@ function DndColumnSelect(props: DndColumnSelectProps) {
return new OptionSelector(optionsMap, multi, value);
}, [multi, options, value]);
// synchronize values in case of dataset changes
const handleOptionsChange = useCallback(() => {
const optionSelectorValues = optionSelector.getValues();
if (typeof value !== typeof optionSelectorValues) {
onChange(optionSelectorValues);
}
if (
typeof value === 'string' &&
typeof optionSelectorValues === 'string' &&
value !== optionSelectorValues
) {
onChange(optionSelectorValues);
}
if (
Array.isArray(optionSelectorValues) &&
Array.isArray(value) &&
(optionSelectorValues.length !== value.length ||
optionSelectorValues.every((val, index) => val === value[index]))
) {
onChange(optionSelectorValues);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [JSON.stringify(value), JSON.stringify(optionSelector.getValues())]);
// useComponentDidUpdate to avoid running this for the first render, to avoid
// calling onChange when the initial value is not valid for the dataset
useComponentDidUpdate(handleOptionsChange);
const onDrop = useCallback(
(item: DatasourcePanelDndItem) => {
const column = item.value as ColumnMeta;
@ -142,8 +115,12 @@ function DndColumnSelect(props: DndColumnSelectProps) {
const valuesRenderer = useCallback(
() =>
optionSelector.values.map((column, idx) =>
clickEnabled ? (
optionSelector.values.map((column, idx) => {
const datasourceWarningMessage =
isAdhocColumn(column) && column.datasourceWarning
? t('This column might be incompatible with current dataset')
: undefined;
return clickEnabled ? (
<ColumnSelectPopoverTrigger
key={idx}
columns={options}
@ -166,6 +143,7 @@ function DndColumnSelect(props: DndColumnSelectProps) {
type={`${DndItemType.ColumnOption}_${name}_${label}`}
canDelete={canDelete}
column={column}
datasourceWarningMessage={datasourceWarningMessage}
withCaret
/>
</ColumnSelectPopoverTrigger>
@ -178,9 +156,10 @@ function DndColumnSelect(props: DndColumnSelectProps) {
type={`${DndItemType.ColumnOption}_${name}_${label}`}
canDelete={canDelete}
column={column}
datasourceWarningMessage={datasourceWarningMessage}
/>
),
),
);
}),
[
canDelete,
clickEnabled,

View File

@ -133,6 +133,7 @@ test('remove selected custom metric when metric gets removed from dataset', () =
);
expect(screen.getByText('metric_a')).toBeVisible();
expect(screen.queryByText('Metric B')).not.toBeInTheDocument();
expect(screen.queryByText('metric_b')).not.toBeInTheDocument();
expect(screen.getByText('SUM(column_a)')).toBeVisible();
expect(screen.getByText('SUM(Column B)')).toBeVisible();
});
@ -171,15 +172,6 @@ test('remove selected custom metric when metric gets removed from dataset for si
],
};
// rerender twice - first to update columns, second to update value
rerender(
<DndMetricSelect
{...newPropsWithRemovedMetric}
value={metricValue}
onChange={onChange}
multi={false}
/>,
);
rerender(
<DndMetricSelect
{...newPropsWithRemovedMetric}
@ -220,15 +212,6 @@ test('remove selected adhoc metric when column gets removed from dataset', async
],
};
// rerender twice - first to update columns, second to update value
rerender(
<DndMetricSelect
{...newPropsWithRemovedColumn}
value={metricValues}
onChange={onChange}
multi
/>,
);
rerender(
<DndMetricSelect
{...newPropsWithRemovedColumn}

View File

@ -22,14 +22,15 @@ import {
ensureIsArray,
FeatureFlag,
GenericDataType,
isAdhocMetricSimple,
isFeatureEnabled,
isSavedMetric,
Metric,
QueryFormMetric,
t,
tn,
} from '@superset-ui/core';
import { ColumnMeta, withDndFallback } from '@superset-ui/chart-controls';
import { isEqual } from 'lodash';
import { usePrevious } from 'src/hooks/usePrevious';
import AdhocMetric from 'src/explore/components/controls/MetricControl/AdhocMetric';
import AdhocMetricPopoverTrigger from 'src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger';
import MetricDefinitionValue from 'src/explore/components/controls/MetricControl/MetricDefinitionValue';
@ -46,24 +47,49 @@ import MetricsControl from '../MetricControl/MetricsControl';
const EMPTY_OBJECT = {};
const DND_ACCEPTED_TYPES = [DndItemType.Column, DndItemType.Metric];
const isDictionaryForAdhocMetric = (value: any) =>
value && !(value instanceof AdhocMetric) && value.expressionType;
const isDictionaryForAdhocMetric = (value: QueryFormMetric) =>
value &&
!(value instanceof AdhocMetric) &&
typeof value !== 'string' &&
value.expressionType;
const coerceAdhocMetrics = (value: any) => {
if (!value) {
const coerceMetrics = (
addedMetrics: QueryFormMetric | QueryFormMetric[] | undefined | null,
savedMetrics: Metric[],
columns: ColumnMeta[],
) => {
if (!addedMetrics) {
return [];
}
if (!Array.isArray(value)) {
if (isDictionaryForAdhocMetric(value)) {
return [new AdhocMetric(value)];
const metricsCompatibleWithDataset = ensureIsArray(addedMetrics).filter(
metric => {
if (isSavedMetric(metric)) {
return savedMetrics.some(
savedMetric => savedMetric.metric_name === metric,
);
}
if (isAdhocMetricSimple(metric)) {
return columns.some(
column => column.column_name === metric.column.column_name,
);
}
return true;
},
);
return metricsCompatibleWithDataset.map(metric => {
if (!isDictionaryForAdhocMetric(metric)) {
return metric;
}
return [value];
}
return value.map(val => {
if (isDictionaryForAdhocMetric(val)) {
return new AdhocMetric(val);
if (isAdhocMetricSimple(metric)) {
const column = columns.find(
col => col.column_name === metric.column.column_name,
);
if (column) {
return new AdhocMetric({ ...metric, column });
}
}
return val;
return new AdhocMetric(metric);
});
};
@ -81,53 +107,8 @@ const getOptionsForSavedMetrics = (
type ValueType = Metric | AdhocMetric | QueryFormMetric;
// TODO: use typeguards to distinguish saved metrics from adhoc metrics
const getMetricsMatchingCurrentDataset = (
values: ValueType[],
columns: ColumnMeta[],
savedMetrics: (savedMetricType | Metric)[],
prevColumns: ColumnMeta[],
prevSavedMetrics: (savedMetricType | Metric)[],
): ValueType[] => {
const areSavedMetricsEqual =
!prevSavedMetrics || isEqual(prevSavedMetrics, savedMetrics);
const areColsEqual = !prevColumns || isEqual(prevColumns, columns);
if (areColsEqual && areSavedMetricsEqual) {
return values;
}
return values.reduce((acc: ValueType[], metric) => {
if (typeof metric === 'string' || (metric as Metric).metric_name) {
if (
areSavedMetricsEqual ||
savedMetrics?.some(
savedMetric =>
savedMetric.metric_name === metric ||
savedMetric.metric_name === (metric as Metric).metric_name,
)
) {
acc.push(metric);
}
return acc;
}
if (!areColsEqual) {
const newCol = columns?.find(
column =>
(metric as AdhocMetric).column?.column_name === column.column_name,
);
if (newCol) {
acc.push({ ...(metric as AdhocMetric), column: newCol });
}
} else {
acc.push(metric);
}
return acc;
}, []);
};
const DndMetricSelect = (props: any) => {
const { onChange, multi, columns, savedMetrics } = props;
const { onChange, multi } = props;
const handleChange = useCallback(
opts => {
@ -153,39 +134,20 @@ const DndMetricSelect = (props: any) => {
);
const [value, setValue] = useState<ValueType[]>(
coerceAdhocMetrics(props.value),
coerceMetrics(props.value, props.savedMetrics, props.columns),
);
const [droppedItem, setDroppedItem] = useState<
DatasourcePanelDndItem | typeof EMPTY_OBJECT
>({});
const [newMetricPopoverVisible, setNewMetricPopoverVisible] = useState(false);
const prevColumns = usePrevious(columns);
const prevSavedMetrics = usePrevious(savedMetrics);
useEffect(() => {
setValue(coerceAdhocMetrics(props.value));
}, [JSON.stringify(props.value)]);
useEffect(() => {
// Remove selected custom metrics that do not exist in the dataset anymore
// Remove selected adhoc metrics that use columns which do not exist in the dataset anymore
// Sync adhoc metrics with dataset columns when they are modified by the user
if (!props.value) {
return;
}
const propsValues = ensureIsArray(props.value);
const matchingMetrics = getMetricsMatchingCurrentDataset(
propsValues,
columns,
savedMetrics,
prevColumns,
prevSavedMetrics,
);
if (!isEqual(propsValues, matchingMetrics)) {
handleChange(matchingMetrics);
}
}, [columns, savedMetrics, handleChange]);
setValue(coerceMetrics(props.value, props.savedMetrics, props.columns));
}, [
JSON.stringify(props.value),
JSON.stringify(props.savedMetrics),
JSON.stringify(props.columns),
]);
const canDrop = useCallback(
(item: DatasourcePanelDndItem) => {
@ -291,6 +253,11 @@ const DndMetricSelect = (props: any) => {
onDropLabel={handleDropLabel}
type={`${DndItemType.AdhocMetricOption}_${props.name}_${props.label}`}
multi={multi}
datasourceWarningMessage={
option instanceof AdhocMetric && option.datasourceWarning
? t('This metric might be incompatible with current dataset')
: undefined
}
/>
),
[

View File

@ -38,6 +38,7 @@ export default function Option({
clickClose,
withCaret,
isExtra,
datasourceWarningMessage,
canDelete = true,
}: OptionProps) {
const theme = useTheme();
@ -60,15 +61,18 @@ export default function Option({
</CloseContainer>
)}
<Label data-test="control-label">{children}</Label>
{isExtra && (
{(!!datasourceWarningMessage || isExtra) && (
<StyledInfoTooltipWithTrigger
icon="exclamation-triangle"
placement="top"
bsStyle="warning"
tooltip={t(`
tooltip={
datasourceWarningMessage ||
t(`
This filter was inherited from the dashboard's context.
It won't be saved when saving the chart.
`)}
`)
}
/>
)}
{withCaret && (

View File

@ -57,6 +57,7 @@ export default function OptionWrapper(
clickClose,
withCaret,
isExtra,
datasourceWarningMessage,
canDelete = true,
...rest
} = props;
@ -176,6 +177,7 @@ export default function OptionWrapper(
clickClose={clickClose}
withCaret={withCaret}
isExtra={isExtra}
datasourceWarningMessage={datasourceWarningMessage}
canDelete={canDelete}
>
<Label />

View File

@ -30,6 +30,7 @@ export interface OptionProps {
clickClose: (index: number) => void;
withCaret?: boolean;
isExtra?: boolean;
datasourceWarningMessage?: string;
canDelete?: boolean;
}

View File

@ -36,6 +36,7 @@ describe('AdhocFilter', () => {
expressionType: EXPRESSION_TYPES.SIMPLE,
subject: 'value',
operator: '>',
datasourceWarning: false,
comparator: '10',
clause: CLAUSES.WHERE,
filterOptionName: adhocFilter.filterOptionName,

View File

@ -118,6 +118,7 @@ export default class AdhocFilter {
}
this.isExtra = !!adhocFilter.isExtra;
this.isNew = !!adhocFilter.isNew;
this.datasourceWarning = !!adhocFilter.datasourceWarning;
this.filterOptionName =
adhocFilter.filterOptionName ||

View File

@ -76,6 +76,7 @@ export default class AdhocMetric {
this.aggregate = null;
}
this.isNew = !!adhocMetric.isNew;
this.datasourceWarning = !!adhocMetric.datasourceWarning;
this.hasCustomLabel = !!(adhocMetric.hasCustomLabel && adhocMetric.label);
this.label = this.hasCustomLabel
? adhocMetric.label

View File

@ -34,6 +34,7 @@ describe('AdhocMetric', () => {
expressionType: EXPRESSION_TYPES.SIMPLE,
column: valueColumn,
aggregate: AGGREGATES.SUM,
datasourceWarning: false,
label: 'SUM(value)',
hasCustomLabel: false,
optionName: adhocMetric.optionName,

View File

@ -38,6 +38,7 @@ const propTypes = {
index: PropTypes.number,
type: PropTypes.string,
multi: PropTypes.bool,
datasourceWarningMessage: PropTypes.string,
};
class AdhocMetricOption extends React.PureComponent {
@ -64,6 +65,7 @@ class AdhocMetricOption extends React.PureComponent {
index,
type,
multi,
datasourceWarningMessage,
} = this.props;
return (
@ -87,6 +89,7 @@ class AdhocMetricOption extends React.PureComponent {
withCaret
isFunction
multi={multi}
datasourceWarningMessage={datasourceWarningMessage}
/>
</AdhocMetricPopoverTrigger>
);

View File

@ -35,6 +35,7 @@ const propTypes = {
savedMetricsOptions: PropTypes.arrayOf(savedMetricType),
multi: PropTypes.bool,
datasource: PropTypes.object,
datasourceWarningMessage: PropTypes.string,
};
export default function MetricDefinitionValue({
@ -50,6 +51,7 @@ export default function MetricDefinitionValue({
index,
type,
multi,
datasourceWarningMessage,
}) {
const getSavedMetricByName = metricName =>
savedMetrics.find(metric => metric.metric_name === metricName);
@ -78,6 +80,7 @@ export default function MetricDefinitionValue({
savedMetric: savedMetric ?? {},
type,
multi,
datasourceWarningMessage,
};
return <AdhocMetricOption {...metricOptionProps} />;

View File

@ -179,6 +179,7 @@ export const OptionControlLabel = ({
type,
index,
isExtra,
datasourceWarningMessage,
tooltipTitle,
multi = true,
...props
@ -195,7 +196,8 @@ export const OptionControlLabel = ({
type: string;
index: number;
isExtra?: boolean;
tooltipTitle: string;
datasourceWarningMessage?: string;
tooltipTitle?: string;
multi?: boolean;
}) => {
const theme = useTheme();
@ -314,15 +316,18 @@ export const OptionControlLabel = ({
{isFunction && <Icons.FieldDerived />}
{getLabelContent()}
</Label>
{isExtra && (
{(!!datasourceWarningMessage || isExtra) && (
<StyledInfoTooltipWithTrigger
icon="exclamation-triangle"
placement="top"
bsStyle="warning"
tooltip={t(`
tooltip={
datasourceWarningMessage ||
t(`
This filter was inherited from the dashboard's context.
It won't be saved when saving the chart.
`)}
`)
}
/>
)}
{withCaret && (

View File

@ -210,6 +210,7 @@ test('SQL ad-hoc metric values', () => {
},
}),
).toEqual({
datasourceWarning: true,
expressionType: 'SQL',
sqlExpression: 'select * from sample_column_1;',
});
@ -279,6 +280,7 @@ test('SQL ad-hoc filter values', () => {
},
}),
).toEqual({
datasourceWarning: true,
expressionType: 'SQL',
sqlExpression: 'select * from sample_column_1;',
});

View File

@ -21,9 +21,9 @@ import { ControlState, Dataset, Metric } from '@superset-ui/chart-controls';
import {
Column,
isAdhocMetricSimple,
isAdhocMetricSQL,
isSavedMetric,
isSimpleAdhocFilter,
isFreeFormAdhocFilter,
JsonValue,
SimpleAdhocFilter,
} from '@superset-ui/core';
@ -72,7 +72,10 @@ const isControlValueCompatibleWithDatasource = (
column.column_name === (value as SimpleAdhocFilter).subject,
);
}
if (isFreeFormAdhocFilter(value)) return true;
if (isAdhocMetricSQL(value)) {
Object.assign(value, { datasourceWarning: true });
return true;
}
return false;
};

View File

@ -54,43 +54,40 @@ export default function exploreReducer(state = {}, action) {
const { prevDatasource, newDatasource } = action;
const controls = { ...state.controls };
const controlsTransferred = [];
if (
prevDatasource.id !== newDatasource.id ||
prevDatasource.type !== newDatasource.type
) {
// reset time range filter to default
newFormData.time_range = DEFAULT_TIME_RANGE;
newFormData.datasource = newDatasource.uid;
// reset control values for column/metric related controls
Object.entries(controls).forEach(([controlName, controlState]) => {
if (
// for direct column select controls
controlState.valueKey === 'column_name' ||
// for all other controls
'savedMetrics' in controlState ||
'columns' in controlState ||
('options' in controlState && !Array.isArray(controlState.options))
) {
controls[controlName] = {
...controlState,
};
newFormData[controlName] = getControlValuesCompatibleWithDatasource(
newDatasource,
controlState,
controlState.value,
);
if (
ensureIsArray(newFormData[controlName]).length > 0 &&
newFormData[controlName] !== controls[controlName].default
) {
controlsTransferred.push(controlName);
}
}
});
}
// reset control values for column/metric related controls
Object.entries(controls).forEach(([controlName, controlState]) => {
if (
// for direct column select controls
controlState.valueKey === 'column_name' ||
// for all other controls
'savedMetrics' in controlState ||
'columns' in controlState ||
('options' in controlState && !Array.isArray(controlState.options))
) {
newFormData[controlName] = getControlValuesCompatibleWithDatasource(
newDatasource,
controlState,
controlState.value,
);
if (
ensureIsArray(newFormData[controlName]).length > 0 &&
newFormData[controlName] !== controls[controlName].default
) {
controlsTransferred.push(controlName);
}
}
});
const newState = {
...state,
controls,

View File

@ -19,7 +19,15 @@
import shortid from 'shortid';
import { compose } from 'redux';
import persistState, { StorageAdapter } from 'redux-localstorage';
import { isEqual, omitBy, isUndefined, isNull } from 'lodash';
import {
isEqual,
omitBy,
omit,
isUndefined,
isNull,
isEqualWith,
} from 'lodash';
import { ensureIsArray } from '@superset-ui/core';
export function addToObject(
state: Record<string, any>,
@ -181,7 +189,8 @@ export function areObjectsEqual(
opts: {
ignoreUndefined?: boolean;
ignoreNull?: boolean;
} = { ignoreUndefined: false, ignoreNull: false },
ignoreFields?: string[];
} = { ignoreUndefined: false, ignoreNull: false, ignoreFields: [] },
) {
let comp1 = obj1;
let comp2 = obj2;
@ -193,5 +202,14 @@ export function areObjectsEqual(
comp1 = omitBy(comp1, isNull);
comp2 = omitBy(comp2, isNull);
}
if (opts.ignoreFields?.length) {
const ignoreFields = ensureIsArray(opts.ignoreFields);
return isEqualWith(comp1, comp2, (val1, val2) =>
isEqual(
ensureIsArray(val1).map(value => omit(value, ignoreFields)),
ensureIsArray(val2).map(value => omit(value, ignoreFields)),
),
);
}
return isEqual(comp1, comp2);
}