diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx index cde7778b29ac..c59e02431fa2 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx @@ -26,9 +26,12 @@ import { } from 'spec/helpers/testing-library'; import { useDroppable } from '@dnd-kit/core'; import { useSortable } from '@dnd-kit/sortable'; -import { DndMetricSelect } from 'src/explore/components/controls/DndColumnSelectControl/DndMetricSelect'; +import { + DndMetricSelect, + coerceMetrics, +} from 'src/explore/components/controls/DndColumnSelectControl/DndMetricSelect'; import { AGGREGATES } from 'src/explore/constants'; -import { EXPRESSION_TYPES } from '../MetricControl/AdhocMetric'; +import AdhocMetric, { EXPRESSION_TYPES } from '../MetricControl/AdhocMetric'; import { DndItemType } from '../../DndItemType'; import { CapturedDroppable, @@ -99,6 +102,39 @@ const adhocMetricB = { optionName: 'def', }; +test('coerceMetrics regenerates duplicate optionNames so each metric stays unique', () => { + // A saved chart can carry two adhoc metrics with the same optionName (e.g. + // born from a duplicated metric). Since edits are matched by optionName, the + // duplicates must be split apart on load or editing one overwrites the other. + const dup = 'shared_option'; + const result = coerceMetrics( + [ + { + expressionType: EXPRESSION_TYPES.SIMPLE, + column: defaultProps.columns[0], + aggregate: AGGREGATES.SUM, + optionName: dup, + }, + { + expressionType: EXPRESSION_TYPES.SIMPLE, + column: defaultProps.columns[1], + aggregate: AGGREGATES.AVG, + optionName: dup, + }, + ] as any, + defaultProps.savedMetrics, + defaultProps.columns, + ) as AdhocMetric[]; + + expect(result).toHaveLength(2); + // First keeps the optionName, second is regenerated to avoid the collision. + expect(result[0].optionName).toBe(dup); + expect(result[1].optionName).not.toBe(dup); + // Each metric definition is otherwise preserved. + expect(result[0].aggregate).toBe(AGGREGATES.SUM); + expect(result[1].aggregate).toBe(AGGREGATES.AVG); +}); + test('renders with default props', () => { render(, { useDndKit: true, diff --git a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx index 5bd4eb5d4c1e..08083e175b34 100644 --- a/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx +++ b/superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx @@ -52,7 +52,7 @@ const isDictionaryForAdhocMetric = (value: QueryFormMetric) => typeof value !== 'string' && value.expressionType; -const coerceMetrics = ( +export const coerceMetrics = ( addedMetrics: QueryFormMetric | QueryFormMetric[] | undefined | null, savedMetrics: Metric[], columns: ColumnMeta[], @@ -71,6 +71,24 @@ const coerceMetrics = ( }, ); + // Metrics are identified by optionName when editing; regenerate any that + // collide so each keeps a unique identity and editing one never overwrites + // another (a saved chart can carry duplicate optionNames, e.g. from a + // duplicated metric). + const seenOptionNames = new Set(); + const dedupeOptionName = (metric: AdhocMetric) => { + if (!seenOptionNames.has(metric.optionName)) { + seenOptionNames.add(metric.optionName); + return metric; + } + const deduped = new AdhocMetric({ + ...(metric as unknown as Record), + optionName: undefined, + }); + seenOptionNames.add(deduped.optionName); + return deduped; + }; + return metricsCompatibleWithDataset.map(metric => { if ( isSavedMetric(metric) && @@ -94,14 +112,18 @@ const coerceMetrics = ( ); if (column) { // Cast entire config object to handle type mismatch between @superset-ui/core and local types - return new AdhocMetric({ - ...(metric as unknown as Record), - column, - } as Record); + return dedupeOptionName( + new AdhocMetric({ + ...(metric as unknown as Record), + column, + } as Record), + ); } } // Cast to unknown first to handle type mismatch between @superset-ui/core and local AdhocMetric - return new AdhocMetric(metric as unknown as Record); + return dedupeOptionName( + new AdhocMetric(metric as unknown as Record), + ); }); }; diff --git a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.tsx b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.tsx index 78db85a6cb41..39598c1489a7 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.tsx +++ b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.tsx @@ -124,6 +124,48 @@ test('accepts an edited metric from an AdhocMetricEditPopover', async () => { ]); }); +test('only edits the targeted metric when two metrics share an optionName', async () => { + // A saved chart can carry two adhoc metrics with the same optionName (e.g. + // born from a duplicated metric). Editing one must not overwrite the other. + // Saved charts store metrics as plain dictionaries in form_data, so mirror + // that shape (not AdhocMetric instances) to exercise the real load path. + const sharedOptionName = 'metric_shared_option'; + const { onChange } = setup({ + value: [ + { + expressionType: EXPRESSION_TYPES.SIMPLE, + column: valueColumn, + aggregate: AGGREGATES.SUM, + label: 'SUM(value)', + optionName: sharedOptionName, + }, + { + expressionType: EXPRESSION_TYPES.SIMPLE, + column: valueColumn, + aggregate: AGGREGATES.AVG, + label: 'AVG(value)', + optionName: sharedOptionName, + }, + ], + }); + + userEvent.click(screen.getByText('SUM(value)')); + await screen.findByText('aggregate'); + await selectOption('MAX', 'Select aggregate options'); + await screen.findByText('MAX(value)'); + userEvent.click(screen.getByRole('button', { name: /save/i })); + + // The untouched AVG(value) metric must still be present and unchanged. + expect(onChange).toHaveBeenCalledWith( + expect.arrayContaining([ + expect.objectContaining({ + aggregate: AGGREGATES.AVG, + label: 'AVG(value)', + }), + ]), + ); +}); + test('removes metrics if savedMetrics changes', async () => { setup({ value: [sumValueAdhocMetric], diff --git a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.tsx b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.tsx index 893bc2119bad..ff09c3b91da3 100644 --- a/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.tsx +++ b/superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.tsx @@ -66,10 +66,20 @@ function coerceAdhocMetrics(value: any) { } return [value]; } + // Metrics are identified by optionName when editing; regenerate any that + // collide so each keeps a unique identity and editing one never overwrites + // another (a saved chart can carry duplicate optionNames, e.g. from a + // duplicated metric). + const seenOptionNames = new Set(); // eslint-disable-next-line @typescript-eslint/no-explicit-any return value.map((val: any) => { if (isDictionaryForAdhocMetric(val)) { - return new AdhocMetric(val); + const metric = + val.optionName && seenOptionNames.has(val.optionName) + ? new AdhocMetric({ ...val, optionName: undefined }) + : new AdhocMetric(val); + seenOptionNames.add(metric.optionName); + return metric; } return val; });