diff --git a/superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx b/superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx index 3f2addf4f70..4aacf585743 100644 --- a/superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx +++ b/superset-frontend/src/SqlLab/components/SaveDatasetModal/SaveDatasetModal.test.tsx @@ -30,7 +30,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, @@ -354,6 +354,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(, { + 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(, { + 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'), diff --git a/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx b/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx index e6ead23fbb3..c3b4ddf4991 100644 --- a/superset-frontend/src/SqlLab/components/SaveDatasetModal/index.tsx +++ b/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>, - owners: [number], - overrideColumns: boolean, -) => { +type UpdateDatasetPayload = { + dbId: number; + datasetId: number; + sql: string; + columns: Array>; + 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; + // 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({