fix(explore): stop metric edits from bleeding across metrics with duplicate optionNames

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.test.tsx

@@ -24,11 +24,15 @@ import {
   waitFor,
   within,
 } from 'spec/helpers/testing-library';
+import { Metric } from '@superset-ui/core';
 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 +103,70 @@ 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 as unknown as Metric[],
+    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('coerceMetrics regenerates duplicate optionNames for SQL adhoc metrics too', () => {
+  // The same collision can happen with custom SQL metrics, which take a
+  // different code path than column-backed metrics but must dedupe the same way.
+  const dup = 'shared_option';
+  const result = coerceMetrics(
+    [
+      {
+        expressionType: EXPRESSION_TYPES.SQL,
+        sqlExpression: 'COUNT(*)',
+        label: 'count',
+        optionName: dup,
+      },
+      {
+        expressionType: EXPRESSION_TYPES.SQL,
+        sqlExpression: 'SUM(value)',
+        label: 'total',
+        optionName: dup,
+      },
+    ] as any,
+    defaultProps.savedMetrics as unknown as Metric[],
+    defaultProps.columns,
+  ) as AdhocMetric[];
+
+  expect(result).toHaveLength(2);
+  expect(result[0].optionName).toBe(dup);
+  expect(result[1].optionName).not.toBe(dup);
+  // Each metric definition is otherwise preserved.
+  expect(result[0].sqlExpression).toBe('COUNT(*)');
+  expect(result[1].sqlExpression).toBe('SUM(value)');
+});
+
 test('renders with default props', () => {
   render(<DndMetricSelect {...defaultProps} />, {
     useDndKit: true,

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<string>();
+  const dedupeOptionName = (metric: AdhocMetric) => {
+    if (!seenOptionNames.has(metric.optionName)) {
+      seenOptionNames.add(metric.optionName);
+      return metric;
+    }
+    const deduped = new AdhocMetric({
+      ...(metric as unknown as Record<string, unknown>),
+      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<string, unknown>),
-          column,
-        } as Record<string, unknown>);
+        return dedupeOptionName(
+          new AdhocMetric({
+            ...(metric as unknown as Record<string, unknown>),
+            column,
+          } as Record<string, unknown>),
+        );
       }
     }
     // Cast to unknown first to handle type mismatch between @superset-ui/core and local AdhocMetric
-    return new AdhocMetric(metric as unknown as Record<string, unknown>);
+    return dedupeOptionName(
+      new AdhocMetric(metric as unknown as Record<string, unknown>),
+    );
   });
 };
 

superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.test.tsx

@@ -124,6 +124,53 @@ 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 edit must propagate to the targeted metric (SUM → MAX) while the
+  // untouched AVG(value) metric stays present and unchanged.
+  expect(onChange).toHaveBeenCalledWith(
+    expect.arrayContaining([
+      expect.objectContaining({
+        aggregate: AGGREGATES.MAX,
+        label: 'MAX(value)',
+      }),
+      expect.objectContaining({
+        aggregate: AGGREGATES.AVG,
+        label: 'AVG(value)',
+      }),
+    ]),
+  );
+});
+
 test('removes metrics if savedMetrics changes', async () => {
   setup({
     value: [sumValueAdhocMetric],

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<string>();
   // 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;
   });