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;
});