refactor(sqllab): address review feedback on SaveDatasetModal

- Convert updateDataset from 7 positional args to a typed options object
  (UpdateDatasetPayload) for readability at call sites.
- Simplify sanitizeTemplateParams to strip _filters via destructuring,
  removing the no-underscore-dangle eslint-disable.
- Harden overwrite tests: locate the dataset PUT call by endpoint rather
  than by call index, and assert override_columns=true.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx

@@ -29,7 +29,7 @@ import fetchMock from 'fetch-mock';
 import { SaveDatasetModal } from 'src/SqlLab/components/SaveDatasetModal';
 import { createDatasource } from 'src/SqlLab/actions/sqlLab';
 import { user, testQuery, mockdatasets } from 'src/SqlLab/fixtures';
-import { FeatureFlag } from '@superset-ui/core';
+import { FeatureFlag, SupersetClient } from '@superset-ui/core';
 
 const mockedProps = {
   visible: true,
@@ -341,6 +341,121 @@ describe('SaveDatasetModal', () => {
     });
   });
 
+  const setupOverwriteFlow = async () => {
+    // Select the "Overwrite existing" radio
+    userEvent.click(screen.getByRole('radio', { name: /overwrite existing/i }));
+    // Open the select and pick an existing dataset
+    userEvent.click(
+      screen.getByRole('combobox', { name: /existing dataset/i }),
+    );
+    await waitFor(() =>
+      expect(screen.queryByText('Loading...')).not.toBeVisible(),
+    );
+    userEvent.click(screen.getAllByText('coolest table 0')[1]);
+    // First overwrite click → confirmation screen
+    userEvent.click(screen.getByRole('button', { name: /overwrite/i }));
+    // Wait for the confirmation screen to render
+    await screen.findByText(/are you sure you want to overwrite this dataset/i);
+    // Second overwrite click → triggers the PUT
+    userEvent.click(screen.getByRole('button', { name: /overwrite/i }));
+  };
+
+  test('sends template_params when overwriting a dataset with include template parameters checked', async () => {
+    // @ts-expect-error
+    global.featureFlags = {
+      [FeatureFlag.EnableTemplateProcessing]: true,
+    };
+
+    const putSpy = jest
+      .spyOn(SupersetClient, 'put')
+      .mockResolvedValue({ json: { result: { id: 0 } } } as any);
+
+    const dummyDispatch = jest.fn().mockResolvedValue({});
+    useDispatchMock.mockReturnValue(dummyDispatch);
+    useSelectorMock.mockReturnValue({ ...user });
+
+    const propsWithTemplateParam = {
+      ...mockedProps,
+      datasource: {
+        ...testQuery,
+        templateParams: JSON.stringify({ my_param: 12, _filters: 'foo' }),
+      },
+    };
+    render(<SaveDatasetModal {...propsWithTemplateParam} />, {
+      useRedux: true,
+    });
+
+    // Check the "Include Template Parameters" checkbox
+    userEvent.click(screen.getByRole('checkbox'));
+
+    await setupOverwriteFlow();
+
+    await waitFor(() => {
+      expect(
+        putSpy.mock.calls.some(([req]) =>
+          req.endpoint?.includes('api/v1/dataset/'),
+        ),
+      ).toBe(true);
+    });
+
+    const datasetPutCall = putSpy.mock.calls.find(([req]) =>
+      req.endpoint?.includes('api/v1/dataset/'),
+    )!;
+    const [req] = datasetPutCall;
+    expect(req.endpoint).toContain('override_columns=true');
+    const body = JSON.parse(req.body as string);
+    // _filters should be stripped, but my_param should be preserved
+    expect(body.template_params).toEqual(JSON.stringify({ my_param: 12 }));
+
+    putSpy.mockRestore();
+  });
+
+  test('does not send template_params when overwriting a dataset with include template parameters unchecked', async () => {
+    // @ts-expect-error
+    global.featureFlags = {
+      [FeatureFlag.EnableTemplateProcessing]: true,
+    };
+
+    const putSpy = jest
+      .spyOn(SupersetClient, 'put')
+      .mockResolvedValue({ json: { result: { id: 0 } } } as any);
+
+    const dummyDispatch = jest.fn().mockResolvedValue({});
+    useDispatchMock.mockReturnValue(dummyDispatch);
+    useSelectorMock.mockReturnValue({ ...user });
+
+    const propsWithTemplateParam = {
+      ...mockedProps,
+      datasource: {
+        ...testQuery,
+        templateParams: JSON.stringify({ my_param: 12 }),
+      },
+    };
+    render(<SaveDatasetModal {...propsWithTemplateParam} />, {
+      useRedux: true,
+    });
+
+    // Do NOT check the "Include Template Parameters" checkbox
+    await setupOverwriteFlow();
+
+    await waitFor(() => {
+      expect(
+        putSpy.mock.calls.some(([req]) =>
+          req.endpoint?.includes('api/v1/dataset/'),
+        ),
+      ).toBe(true);
+    });
+
+    const datasetPutCall = putSpy.mock.calls.find(([req]) =>
+      req.endpoint?.includes('api/v1/dataset/'),
+    )!;
+    const [req] = datasetPutCall;
+    const body = JSON.parse(req.body as string);
+    expect(body.template_params).toBeUndefined();
+
+    putSpy.mockRestore();
+  });
+
   test('clears dataset cache when creating new dataset', async () => {
     const clearDatasetCache = jest.spyOn(
       require('src/utils/cachedSupersetGet'),

superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx

@@ -149,14 +149,25 @@ const Styles = styled.div`
     }
   `}
 `;
-const updateDataset = async (
-  dbId: number,
-  datasetId: number,
-  sql: string,
-  columns: Array<Record<string, any>>,
-  owners: [number],
-  overrideColumns: boolean,
-) => {
+type UpdateDatasetPayload = {
+  dbId: number;
+  datasetId: number;
+  sql: string;
+  columns: Array<Record<string, any>>;
+  owners: number[];
+  overrideColumns: boolean;
+  templateParams?: string;
+};
+
+const updateDataset = async ({
+  dbId,
+  datasetId,
+  sql,
+  columns,
+  owners,
+  overrideColumns,
+  templateParams,
+}: UpdateDatasetPayload) => {
   const endpoint = `api/v1/dataset/${datasetId}?override_columns=${overrideColumns}`;
   const headers = { 'Content-Type': 'application/json' };
   const body = JSON.stringify({
@@ -164,6 +175,7 @@ const updateDataset = async (
     columns,
     owners,
     database_id: dbId,
+    ...(templateParams !== undefined && { template_params: templateParams }),
   });
 
   const data: JsonResponse = await SupersetClient.put({
@@ -179,6 +191,26 @@ const updateDataset = async (
 
 const UNTITLED = t('Untitled Dataset');
 
+// The filters param is only used to test jinja templates.
+// Remove the special filters entry from the templateParams
+// before saving the dataset.
+const sanitizeTemplateParams = (
+  templateParams: string | object | null | undefined,
+): string | undefined => {
+  if (typeof templateParams !== 'string') {
+    return undefined;
+  }
+  try {
+    const parsed = JSON.parse(templateParams) as Record<string, unknown>;
+    // Remove the special _filters entry — it is only used to test jinja templates.
+    const { _filters: _ignored, ...clean } = parsed;
+    return JSON.stringify(clean);
+  } catch (e) {
+    // malformed templateParams, do not include it
+    return undefined;
+  }
+};
+
 export const SaveDatasetModal = ({
   visible,
   onHide,
@@ -232,22 +264,27 @@ export const SaveDatasetModal = ({
     }
     setLoading(true);
 
+    const templateParams = includeTemplateParameters
+      ? sanitizeTemplateParams(datasource?.templateParams)
+      : undefined;
+
     try {
       const [, key] = await Promise.all([
-        updateDataset(
-          datasource?.dbId,
-          datasetToOverwrite?.datasetid,
-          datasource?.sql,
-          datasource?.columns?.map(
+        updateDataset({
+          dbId: datasource?.dbId,
+          datasetId: datasetToOverwrite?.datasetid,
+          sql: datasource?.sql,
+          columns: datasource?.columns?.map(
             (d: { column_name: string; type: string; is_dttm: boolean }) => ({
               column_name: d.column_name,
               type: d.type,
               is_dttm: d.is_dttm,
             }),
           ),
-          datasetToOverwrite?.owners?.map((o: DatasetOwner) => o.id),
-          true,
-        ),
+          owners: datasetToOverwrite?.owners?.map((o: DatasetOwner) => o.id),
+          overrideColumns: true,
+          templateParams,
+        }),
         postFormData(datasetToOverwrite.datasetid, 'table', {
           ...formDataWithDefaults,
           datasource: `${datasetToOverwrite.datasetid}__table`,
@@ -319,27 +356,9 @@ export const SaveDatasetModal = ({
     setLoading(true);
     const selectedColumns = datasource?.columns ?? [];
 
-    // The filters param is only used to test jinja templates.
-    // Remove the special filters entry from the templateParams
-    // before saving the dataset.
-    let templateParams;
-    if (
-      typeof datasource?.templateParams === 'string' &&
-      includeTemplateParameters
-    ) {
-      try {
-        const p = JSON.parse(datasource.templateParams);
-        /* eslint-disable-next-line no-underscore-dangle */
-        if (p._filters) {
-          /* eslint-disable-next-line no-underscore-dangle */
-          delete p._filters;
-        }
-        templateParams = JSON.stringify(p);
-      } catch (e) {
-        // malformed templateParams, do not include it
-        templateParams = undefined;
-      }
-    }
+    const templateParams = includeTemplateParameters
+      ? sanitizeTemplateParams(datasource?.templateParams)
+      : undefined;
 
     dispatch(
       createDatasource({