diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index d543833b9df4..b426f053819c 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -78,6 +78,6 @@ jobs: # queries: security-extended,security-and-quality - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@db488ddef3bf6cb639b32c2e9a7c0a7ea8271d28 # v4.37.8 + uses: github/codeql-action/analyze@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 with: category: "/language:${{matrix.language}}" diff --git a/UPDATING.md b/UPDATING.md index 1c582a081b4d..4186ca88a8b2 100644 --- a/UPDATING.md +++ b/UPDATING.md @@ -24,6 +24,21 @@ assists people when migrating to a new version. ## Next +### Archived dataset purge requires impact confirmation + +`GET /api/v1/dataset//purge-impact` returns the charts and distinct +dashboards affected by permanently deleting an archived dataset, together with +an opaque `impact_token`. The dataset purge endpoint now requires that token in +the JSON body as `confirmed_impact_token`. API clients that call +`POST /api/v1/dataset//purge` must fetch and display the impact first; +requests with a missing or malformed token are rejected with 400. + +The server rechecks the dependency identities immediately before mutation. If +they changed, purge performs no deletion and returns 409 with a refreshed impact +payload. Clients must display the new impact and obtain renewed confirmation +before retrying. Preview or recheck failures fail closed rather than treating +unknown impact as zero. Chart and dashboard purge endpoints are unchanged. + - `SAMPLES_ROW_LIMIT` is now the default for `/datasource/samples` requests without a valid explicit `per_page`, rather than a hard per-request ceiling; explicit limits are honored up to the existing global row-limit ceiling, matching `/chart/data` SAMPLES requests. - The `cockroachdb` extra (`pip install apache-superset[cockroachdb]`) now installs `sqlalchemy-cockroachdb` instead of the abandoned `cockroachdb` package, whose SQLAlchemy dialect could not be imported under SQLAlchemy 2.0. Existing environments with the old package installed should `pip uninstall cockroachdb && pip install sqlalchemy-cockroachdb` (or simply reinstall the extra) to restore CockroachDB connectivity. diff --git a/docs/docs/using-superset/recently-archived.mdx b/docs/docs/using-superset/recently-archived.mdx index ebca8c6c4160..f000bbc9311a 100644 --- a/docs/docs/using-superset/recently-archived.mdx +++ b/docs/docs/using-superset/recently-archived.mdx @@ -70,5 +70,24 @@ alert or report is removed, and the reason is shown. Charts that belong to dashboards are removed from those dashboards as part of the deletion; the dashboards themselves are left in place. +Before an archived dataset is deleted permanently, Superset checks which +charts still use it and which dashboards contain those charts. The confirmation +shows the total number of affected charts and dashboards, identifies the ones +you are allowed to access, and reports the remaining objects only as restricted +counts. Restricted names, identifiers, and links are not displayed. Archived +dependents are included because they can still be recovered after the dataset +is gone. + +Deleting the dataset does not delete those charts or dashboards. They remain +in place without a usable dataset and may therefore be broken. If there are no +dependents, the confirmation explicitly reports zero affected charts and +dashboards. + +The dependency check fails closed. While it is loading, or if its result is +unavailable, permanent deletion is disabled; cancel or retry the check. Superset +checks again when you submit. If dependencies changed while the confirmation +was open, the refreshed impact replaces the previous result and you must type +DELETE again before proceeding. + Objects are also deleted permanently on their own once they have been in the archive longer than the retention window, without anyone acting. diff --git a/superset-embedded-sdk/package-lock.json b/superset-embedded-sdk/package-lock.json index 41ee4e641dde..8ff29fe5734a 100644 --- a/superset-embedded-sdk/package-lock.json +++ b/superset-embedded-sdk/package-lock.json @@ -3237,9 +3237,9 @@ "dev": true }, "node_modules/fast-uri": { - "version": "3.1.5", - "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.5.tgz", - "integrity": "sha512-gHwA1O9LDIcKunMKhObS/HimwtehO1nPUECKAu5TpKgaO19fcWEl4bliWe1jWxVFvIXztJjjQ4L8XQ1EU9f7Jw==", + "version": "3.1.7", + "resolved": "https://registry.npmjs.org/fast-uri/-/fast-uri-3.1.7.tgz", + "integrity": "sha512-dOvZVzjdZdz7phd9v6jCbwxrBW3fK6n8Rc0CtdmM4bumzMnxywBYhuph6J819RRw/ku+rLbelwfMunktuzVVHg==", "dev": true, "funding": [ { diff --git a/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/DeleteModal.test.tsx b/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/DeleteModal.test.tsx index 8cb780db9794..d3b456b30162 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/DeleteModal.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/DeleteModal.test.tsx @@ -146,3 +146,61 @@ test('Calling "onConfirm" only after typing "delete" in the input', async () => // confirm input has been cleared expect(screen.getByTestId('delete-modal-input')).toHaveValue(''); }); + +test('external disable keeps the destructive action unavailable after confirmation', async () => { + const onConfirm = jest.fn(); + render( + , + ); + + await userEvent.type(screen.getByTestId('delete-modal-input'), 'DELETE'); + + expect(screen.getByRole('button', { name: 'Delete' })).toBeDisabled(); + await userEvent.click(screen.getByRole('button', { name: 'Delete' })); + expect(onConfirm).not.toHaveBeenCalled(); +}); + +test('loading disables the destructive action and exposes busy state', async () => { + render( + , + ); + + await userEvent.type(screen.getByTestId('delete-modal-input'), 'DELETE'); + + expect(screen.getByTestId('modal-confirm-button')).toBeDisabled(); + expect(screen.getByTestId('antd-modal')).toHaveAttribute('aria-busy', 'true'); +}); + +test('confirmation reset key clears and re-arms type-to-confirm', async () => { + const props = { + title: 'Delete permanently?', + description: 'This cannot be undone.', + onConfirm: jest.fn(), + onHide: jest.fn(), + open: true, + }; + const { rerender } = render( + , + ); + await userEvent.type(screen.getByTestId('delete-modal-input'), 'DELETE'); + expect(screen.getByRole('button', { name: 'Delete' })).toBeEnabled(); + + rerender(); + + expect(screen.getByTestId('delete-modal-input')).toHaveValue(''); + expect(screen.getByRole('button', { name: 'Delete' })).toBeDisabled(); +}); diff --git a/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/index.tsx b/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/index.tsx index 28d04c2be694..e9fc9b8bebcd 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/index.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/index.tsx @@ -40,6 +40,9 @@ export function DeleteModal({ title, name, recoverable = false, + disablePrimaryButton = false, + loading = false, + confirmationResetKey, }: DeleteModalProps) { // Recoverable (archive) deletes drop the "type DELETE to confirm" step; // a permanent delete keeps it. @@ -54,6 +57,11 @@ export function DeleteModal({ } }, [open]); + useEffect(() => { + setConfirmation(''); + setDisableChange(true); + }, [confirmationResetKey]); + // Re-arm the gate alongside clearing the text: resetting only the string // leaves disableChange=false behind, so a user who typed DELETE, cancelled, // and reopened would face an enabled Delete button over an empty input. @@ -76,14 +84,19 @@ export function DeleteModal({ }; const onPressEnter = () => { - if (!disableChange) { + if (!disableChange && !disablePrimaryButton && !loading) { confirm(); } }; return ( {description} diff --git a/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/types.ts b/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/types.ts index 20be0f06e67e..445e5d6d0041 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/types.ts +++ b/superset-frontend/packages/superset-ui-core/src/components/DeleteModal/types.ts @@ -32,4 +32,10 @@ export interface DeleteModalProps { * friction and uses a primary (non-danger) confirm button. */ recoverable?: boolean; + /** Disable confirmation independently of the typed-text gate. */ + disablePrimaryButton?: boolean; + /** Show progress on the primary action and prevent duplicate submission. */ + loading?: boolean; + /** Clear and re-arm the typed-text gate when the reviewed data changes. */ + confirmationResetKey?: string | number; } diff --git a/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx b/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx index b7e61498e676..cdb10f7744e7 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.test.tsx @@ -752,6 +752,97 @@ test('displays an error message when an exception is thrown while fetching', asy expect(screen.getByText(error)).toBeInTheDocument(); }); +test('clears a previous fetch error once a later fetch succeeds', async () => { + const error = 'Fetch error'; + const loadOptions = jest.fn(async (search: string) => { + if (search === 'fail') { + throw new Error(error); + } + // Report more results than are loaded so every new search hits the server. + return { data: [{ label: search, value: search }], totalCount: 100 }; + }); + render(); + await open(); + await type('fail'); + expect(await screen.findByText(error)).toBeInTheDocument(); + + await type('retry'); + expect(await findSelectOption('retry')).toBeInTheDocument(); + expect(screen.queryByText(error)).not.toBeInTheDocument(); +}); + +test('clears a previous fetch error when the next page comes from cache', async () => { + const error = 'Fetch error'; + const loadOptions = jest.fn( + async (search: string, page: number, pageSize: number) => { + if (search === 'fail') { + throw new Error(error); + } + return defaultProps.options(search, page, pageSize); + }, + ); + render(); + await open(); + await findSelectOption(OPTIONS[0].label); + + await type('fail'); + expect(await screen.findByText(error)).toBeInTheDocument(); + + // Clearing the input re-requests the first page, which is already cached + // and therefore never reaches the network. + await userEvent.clear(getSelect()); + expect(await findSelectOption(OPTIONS[0].label)).toBeInTheDocument(); + expect(screen.queryByText(error)).not.toBeInTheDocument(); +}); + +test('ignores a late failure from a search the user has moved on from', async () => { + const error = 'Fetch error'; + let rejectSlow: (reason: Error) => void = () => {}; + const loadOptions = jest.fn(async (search: string) => { + if (search === 'slow') { + return new Promise((_, reject) => { + rejectSlow = reject; + }); + } + return { data: [{ label: search, value: search }], totalCount: 100 }; + }); + render(); + await open(); + await type('slow'); + await waitFor(() => expect(loadOptions).toHaveBeenCalledWith('slow', 0, 10)); + + await type('fast'); + expect(await findSelectOption('fast')).toBeInTheDocument(); + + rejectSlow(new Error(error)); + await waitFor(() => expect(loadOptions).toHaveBeenCalledTimes(3)); + expect(screen.queryByText(error)).not.toBeInTheDocument(); + expect(await findSelectOption('fast')).toBeInTheDocument(); +}); + +test('still surfaces a base-fetch failure that lands mid-search', async () => { + const error = 'Fetch error'; + let rejectBase: (reason: Error) => void = () => {}; + const loadOptions = jest.fn(async (search: string) => { + if (search === '') { + // Defer the base page so it can fail after the user starts searching. + return new Promise((_, reject) => { + rejectBase = reject; + }); + } + return { data: [{ label: search, value: search }], totalCount: 100 }; + }); + render(); + await open(); + await type('abc'); + expect(await findSelectOption('abc')).toBeInTheDocument(); + + // Base fetches keep the accumulator and allValuesLoaded up to date even + // mid-search, so their failures must surface too. + rejectBase(new Error(error)); + expect(await screen.findByText(error)).toBeInTheDocument(); +}); + test('does not fire a new request for the same search input', async () => { const loadOptions = jest.fn(async () => ({ data: [], totalCount: 0 })); render( diff --git a/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx b/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx index 61bcbe80f03f..6174aed43ca8 100644 --- a/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx +++ b/superset-frontend/packages/superset-ui-core/src/components/Select/AsyncSelect.tsx @@ -401,6 +401,10 @@ const AsyncSelect = forwardRef( const fetchPage = useMemo( () => (search: string, page: number) => { setPage(page); + // A previous fetch may have left an error on screen. Clear it before + // any early return so a page served from cache, or from an already + // complete option set, is not shown next to a stale error. + setError(''); if (allValuesLoaded) { setIsLoading(false); return; @@ -486,7 +490,19 @@ const AsyncSelect = forwardRef( setTotalCount(totalCount); } }) - .catch(internalOnError) + .catch((response: Response) => { + // Mirror the results guard above: a failure belonging to a search + // the user has since moved on from must not replace the outcome + // of the fetch that superseded it. Base fetches (search === '') + // are exempt exactly as their results are — they maintain the + // accumulator and allValuesLoaded, so their failures must stay + // visible even when they land mid-search. The consumer's onError + // is skipped along with the banner for superseded searches. + if (search && inputValueRef.current !== search) { + return undefined; + } + return internalOnError(response); + }) .finally(() => { inFlightFetchesRef.current = Math.max( 0, diff --git a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx index fa63e8b40a57..5752969e7df3 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal/DatasourceModal.test.tsx @@ -22,10 +22,13 @@ import { waitFor, fireEvent, cleanup, + userEvent, + act, defaultStore as store, } from 'spec/helpers/testing-library'; import fetchMock from 'fetch-mock'; import { SupersetClient } from '@superset-ui/core'; +import { Constants } from '@superset-ui/core/components'; import mockDatasource from 'spec/fixtures/mockDatasource'; import React from 'react'; import DatasourceModalComponent, { buildExtraJsonObject } from '.'; @@ -86,6 +89,10 @@ beforeEach(async () => { await waitForSaveEnabled(); }); +afterEach(() => { + jest.useRealTimers(); +}); + // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks describe('DatasourceModal', () => { test('renders', async () => { @@ -145,6 +152,93 @@ describe('DatasourceModal', () => { expect(JSON.parse(putCall?.options?.body as string).editors).toEqual([1]); }); + test('saves dataset certification from Settings without dropping Extra metadata', async () => { + cleanup(); + renderAndWait({ + ...mockedProps, + datasource: { + ...mockedProps.datasource, + extra: JSON.stringify({ + custom_key: { enabled: true }, + warning_markdown: 'Use only finalized records', + }), + } as typeof mockedProps.datasource & { extra: string }, + }); + + await userEvent.click(await screen.findByRole('tab', { name: 'Settings' })); + + const defaultUrlLabel = await screen.findByText('Default URL'); + const defaultUrl = defaultUrlLabel + .closest('.ant-form-item') + ?.querySelector('input'); + expect(defaultUrl).not.toBeNull(); + const certifiedBy = await screen.findByPlaceholderText('Certified by'); + const details = screen.getByPlaceholderText('Certification details'); + + jest.useFakeTimers(); + fireEvent.change(defaultUrl as HTMLInputElement, { + target: { value: '/dashboard/7/' }, + }); + fireEvent.change(certifiedBy, { target: { value: 'E2E Team' } }); + fireEvent.change(details, { + target: { value: 'Reviewed for production' }, + }); + act(() => { + jest.advanceTimersByTime(Constants.FAST_DEBOUNCE); + }); + jest.useRealTimers(); + + fireEvent.click(screen.getByTestId('datasource-modal-save')); + fireEvent.click(await screen.findByRole('button', { name: 'Confirm' })); + + await waitFor(() => { + const putCall = fetchMock.callHistory + .calls() + .find( + call => + call.url.includes('/api/v1/dataset/7') && + call.options?.method === 'put', + ); + expect(putCall).toBeDefined(); + + const payload = JSON.parse(putCall?.options?.body as string); + expect(payload.default_endpoint).toBe('/dashboard/7/'); + expect(JSON.parse(payload.extra)).toEqual({ + custom_key: { enabled: true }, + warning_markdown: 'Use only finalized records', + certification: { + certified_by: 'E2E Team', + details: 'Reviewed for production', + }, + }); + }); + }); + + test('shows existing dataset certification in Settings', async () => { + cleanup(); + renderAndWait({ + ...mockedProps, + datasource: { + ...mockedProps.datasource, + extra: JSON.stringify({ + certification: { + certified_by: 'Data Platform Team', + details: 'Source of truth', + }, + }), + } as typeof mockedProps.datasource & { extra: string }, + }); + + await userEvent.click(await screen.findByRole('tab', { name: 'Settings' })); + + expect(await screen.findByPlaceholderText('Certified by')).toHaveValue( + 'Data Platform Team', + ); + expect(screen.getByPlaceholderText('Certification details')).toHaveValue( + 'Source of truth', + ); + }); + test('should render error dialog', async () => { const putSpy = jest .spyOn(SupersetClient, 'put') diff --git a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx index ae296f9c9fa1..d70a1ad2c34a 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx +++ b/superset-frontend/src/components/Datasource/DatasourceModal/index.tsx @@ -43,6 +43,7 @@ import type { DatasetObject } from 'src/features/datasets/types'; import { withCertificationFields } from '../utils'; import { mapSubjectValuesToIds } from 'src/features/subjects/SubjectPicker'; import type { DatasourceModalProps } from '../types'; +import { setDatasetCertification } from '../components/DatasourceEditor/datasetCertification'; const DatasourceEditor = AsyncEsmComponent( () => import('../components/DatasourceEditor'), @@ -188,7 +189,12 @@ const DatasourceModal: FunctionComponent = ({ datasource.cache_timeout === '' ? null : datasource.cache_timeout, is_sqllab_view: datasource.is_sqllab_view, template_params: datasource.template_params, - extra: datasource.extra, + extra: datasource.dataset_certification_changed + ? setDatasetCertification(datasource.extra, { + certified_by: datasource.certified_by, + certification_details: datasource.certification_details, + }) + : datasource.extra, is_managed_externally: datasource.is_managed_externally, external_url: datasource.external_url, metrics: datasource?.metrics?.map( diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx index 394806b17169..fde4243e9c47 100644 --- a/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx @@ -109,6 +109,10 @@ import { } from '../../FoldersEditor/treeUtils'; import FoldersEditor from '../../FoldersEditor'; import { DatasourceFolder } from 'src/explore/components/DatasourcePanel/types'; +import { + getDatasetCertification, + isDatasetExtraValid, +} from './datasetCertification'; const extensionsRegistry = getExtensionsRegistry(); @@ -185,6 +189,9 @@ interface DatasourceObject { description?: string; default_endpoint?: string; extra?: string; + certified_by?: string; + certification_details?: string; + dataset_certification_changed?: boolean; datasource_type?: string; type?: string; offset?: number; @@ -972,6 +979,7 @@ function DatasourceEditor({ // Initialize datasource state with transformed editors and metrics const [datasource, setDatasource] = useState(() => ({ ...propsDatasource, + ...getDatasetCertification(propsDatasource.extra), editors: normalizeSubjectsToPickerValues(propsDatasource.editors || []), metrics: propsDatasource.metrics?.map(hydrateMetricExtra), })); @@ -1704,12 +1712,67 @@ function DatasourceEditor({ onDatasourceChange, ]); + const renderCertificationFieldset = useCallback(() => { + const certificationError = !isDatasetExtraValid(datasource.extra) + ? t('Fix the Extra JSON to edit certification') + : undefined; + + return isSqla ? ( +
{ + if ( + fieldKey !== 'certified_by' && + fieldKey !== 'certification_details' + ) { + return; + } + setDatasource(previousDatasource => ({ + ...previousDatasource, + [fieldKey]: typeof value === 'string' ? value : undefined, + dataset_certification_changed: true, + })); + }} + > + + } + /> + + } + /> +
+ ) : null; + }, [datasource, isSqla]); + const renderSettingsFieldset = useCallback( () => (
+ onDatasourcePropChange(String(fieldKey), value) + } > )} + { + onDatasourcePropChange('editors', newEditors); + }} + /> {isSqla && ( )} - { - onDatasourceChange({ ...datasource, editors: newEditors }); - }} - />
), - [datasource, onDatasourceChange, isSqla], + [datasource, onDatasourcePropChange, isSqla], ); const renderAdvancedFieldset = useCallback( @@ -1799,7 +1861,9 @@ function DatasourceEditor({
+ onDatasourcePropChange(String(fieldKey), value) + } >
), - [datasource, onDatasourceChange, isSqla], + [datasource, onDatasourcePropChange, isSqla], ); const renderSourceFieldset = useCallback( @@ -2621,7 +2685,10 @@ function DatasourceEditor({ children: ( - {renderSettingsFieldset()} + + {renderSettingsFieldset()} + {renderCertificationFieldset()} + {renderAdvancedFieldset()} @@ -2651,6 +2718,7 @@ function DatasourceEditor({ folders, folderCount, handleFoldersChange, + renderCertificationFieldset, renderSettingsFieldset, renderAdvancedFieldset, // `renderSpatialTab` is intentionally retained (see its definition above) diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/datasetCertification.ts b/superset-frontend/src/components/Datasource/components/DatasourceEditor/datasetCertification.ts new file mode 100644 index 000000000000..9034822b2892 --- /dev/null +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/datasetCertification.ts @@ -0,0 +1,111 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +export type DatasetCertification = { + certified_by?: string; + certification_details?: string; +}; + +type JsonObject = Record; + +const isJsonObject = (value: unknown): value is JsonObject => + typeof value === 'object' && value !== null && !Array.isArray(value); + +const parseExtra = (extra?: string): JsonObject | undefined => { + if (!extra?.trim()) { + return {}; + } + + try { + const parsed: unknown = JSON.parse(extra); + return isJsonObject(parsed) ? parsed : undefined; + } catch { + return undefined; + } +}; + +export const isDatasetExtraValid = (extra?: string): boolean => + parseExtra(extra) !== undefined; + +export const getDatasetCertification = ( + extra?: string, +): DatasetCertification => { + const certification = parseExtra(extra)?.certification; + if (!isJsonObject(certification)) { + return {}; + } + + return { + certified_by: + typeof certification.certified_by === 'string' + ? certification.certified_by + : undefined, + certification_details: + typeof certification.details === 'string' + ? certification.details + : undefined, + }; +}; + +export const setDatasetCertification = ( + extra: string | undefined, + { certified_by, certification_details }: DatasetCertification, +): string | undefined => { + const parsedExtra = parseExtra(extra); + + // Do not replace malformed raw metadata while the user is correcting it in + // the adjacent Extra editor. + if (!parsedExtra) { + return extra; + } + + const normalizedCertifiedBy = certified_by || undefined; + const normalizedDetails = certification_details || undefined; + const existing = getDatasetCertification(extra); + + // Avoid reformatting raw Extra JSON when the certification did not change. + if ( + existing.certified_by === normalizedCertifiedBy && + existing.certification_details === normalizedDetails + ) { + return extra; + } + + const existingCertification = parsedExtra.certification; + const certification = isJsonObject(existingCertification) + ? { ...existingCertification } + : {}; + delete certification.certified_by; + delete certification.details; + + if (normalizedCertifiedBy) { + certification.certified_by = normalizedCertifiedBy; + } + if (normalizedDetails) { + certification.details = normalizedDetails; + } + + if (Object.keys(certification).length > 0) { + parsedExtra.certification = certification; + } else { + delete parsedExtra.certification; + } + + return JSON.stringify(parsedExtra); +}; diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorDatasetCertification.test.tsx b/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorDatasetCertification.test.tsx new file mode 100644 index 000000000000..76ae25cbd539 --- /dev/null +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorDatasetCertification.test.tsx @@ -0,0 +1,109 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import fetchMock from 'fetch-mock'; +import { Constants } from '@superset-ui/core/components'; +import { + act, + fireEvent, + screen, + userEvent, + waitFor, +} from 'spec/helpers/testing-library'; +import { + cleanupAsyncOperations, + createProps, + DATASOURCE_ENDPOINT, + dismissDatasourceWarning, + fastRender, + setupDatasourceEditorMocks, +} from './DatasourceEditor.test.utils'; + +beforeEach(() => { + fetchMock.get(DATASOURCE_ENDPOINT, [], { name: DATASOURCE_ENDPOINT }); + setupDatasourceEditorMocks(); +}); + +afterEach(async () => { + jest.useRealTimers(); + await cleanupAsyncOperations(); + fetchMock.clearHistory().removeRoutes(); +}); + +test('a trailing Basic edit keeps pending Certification fields', async () => { + const testProps = createProps(); + const extra = '{ "custom_key": true }'; + testProps.datasource.extra = extra; + fastRender(testProps); + await dismissDatasourceWarning(); + + await userEvent.click(await screen.findByRole('tab', { name: 'Settings' })); + + const defaultUrlLabel = await screen.findByText('Default URL'); + const defaultUrl = defaultUrlLabel + .closest('.ant-form-item') + ?.querySelector('input'); + expect(defaultUrl).not.toBeNull(); + const certifiedBy = await screen.findByPlaceholderText('Certified by'); + const certificationDetails = screen.getByPlaceholderText( + 'Certification details', + ); + + jest.useFakeTimers(); + fireEvent.change(certifiedBy, { + target: { value: 'Data Team' }, + }); + fireEvent.change(certificationDetails, { + target: { value: 'Reviewed for production' }, + }); + fireEvent.change(defaultUrl as HTMLInputElement, { + target: { value: '/dashboard/7/' }, + }); + act(() => { + jest.advanceTimersByTime(Constants.FAST_DEBOUNCE); + }); + jest.useRealTimers(); + + await waitFor(() => { + const { calls } = testProps.onChange.mock; + expect(calls[calls.length - 1]?.[0]).toEqual( + expect.objectContaining({ + default_endpoint: '/dashboard/7/', + certified_by: 'Data Team', + certification_details: 'Reviewed for production', + dataset_certification_changed: true, + extra, + }), + ); + }); +}); + +test('malformed Extra disables dataset certification controls', async () => { + const testProps = createProps(); + testProps.datasource.extra = '{"custom_key":'; + fastRender(testProps); + await dismissDatasourceWarning(); + + await userEvent.click(await screen.findByRole('tab', { name: 'Settings' })); + + expect(await screen.findByPlaceholderText('Certified by')).toBeDisabled(); + expect(screen.getByPlaceholderText('Certification details')).toBeDisabled(); + expect( + screen.getAllByText('Fix the Extra JSON to edit certification'), + ).toHaveLength(2); +}); diff --git a/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/datasetCertification.test.ts b/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/datasetCertification.test.ts new file mode 100644 index 000000000000..5938595dd59f --- /dev/null +++ b/superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/datasetCertification.test.ts @@ -0,0 +1,169 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { + getDatasetCertification, + isDatasetExtraValid, + setDatasetCertification, +} from '../datasetCertification'; + +test('reads dataset certification from Extra JSON', () => { + expect( + getDatasetCertification( + JSON.stringify({ + certification: { + certified_by: 'Data Platform Team', + details: 'Source of truth', + }, + }), + ), + ).toEqual({ + certified_by: 'Data Platform Team', + certification_details: 'Source of truth', + }); +}); + +test('writes dataset certification without discarding other Extra metadata', () => { + const result = setDatasetCertification( + JSON.stringify({ + custom_key: { enabled: true }, + warning_markdown: 'Use only finalized records', + }), + { + certified_by: 'E2E Team', + certification_details: 'Reviewed for production', + }, + ); + + expect(JSON.parse(result ?? '')).toEqual({ + custom_key: { enabled: true }, + warning_markdown: 'Use only finalized records', + certification: { + certified_by: 'E2E Team', + details: 'Reviewed for production', + }, + }); +}); + +test('writes certification details without requiring a certifier', () => { + const result = setDatasetCertification('{}', { + certification_details: 'Reviewed for production', + }); + + expect(JSON.parse(result ?? '')).toEqual({ + certification: { details: 'Reviewed for production' }, + }); +}); + +test('editing certification preserves unknown certification metadata', () => { + const result = setDatasetCertification( + JSON.stringify({ + certification: { + certified_by: 'Data Platform Team', + details: 'Source of truth', + expires_at: '2030-01-01', + }, + }), + { + certified_by: 'E2E Team', + certification_details: 'Reviewed for production', + }, + ); + + expect(JSON.parse(result ?? '')).toEqual({ + certification: { + certified_by: 'E2E Team', + details: 'Reviewed for production', + expires_at: '2030-01-01', + }, + }); +}); + +test('clearing dataset certification preserves other Extra metadata', () => { + const result = setDatasetCertification( + JSON.stringify({ + certification: { + certified_by: 'Data Platform Team', + details: 'Source of truth', + }, + warning_markdown: 'Use only finalized records', + }), + { certified_by: '', certification_details: '' }, + ); + + expect(JSON.parse(result ?? '')).toEqual({ + warning_markdown: 'Use only finalized records', + }); +}); + +test('clearing certification preserves unknown certification metadata', () => { + const result = setDatasetCertification( + JSON.stringify({ + certification: { + certified_by: 'Data Platform Team', + details: 'Source of truth', + expires_at: '2030-01-01', + }, + }), + { certified_by: '', certification_details: '' }, + ); + + expect(JSON.parse(result ?? '')).toEqual({ + certification: { expires_at: '2030-01-01' }, + }); +}); + +test('an unchanged certification leaves Extra formatting untouched', () => { + const extra = '{\n "certification": { "certified_by": "Data Team" }\n}'; + + expect(setDatasetCertification(extra, { certified_by: 'Data Team' })).toBe( + extra, + ); + expect( + setDatasetCertification(undefined, { + certified_by: '', + certification_details: '', + }), + ).toBeUndefined(); +}); + +test('handles non-object Extra and certification values', () => { + expect(getDatasetCertification('[]')).toEqual({}); + expect(getDatasetCertification('{"certification":true}')).toEqual({}); + expect( + setDatasetCertification('{"certification":true}', { + certified_by: 'Data Team', + }), + ).toBe('{"certification":{"certified_by":"Data Team"}}'); +}); + +test('identifies malformed and non-object Extra JSON', () => { + expect(isDatasetExtraValid()).toBe(true); + expect(isDatasetExtraValid('{}')).toBe(true); + expect(isDatasetExtraValid('{"custom_key":')).toBe(false); + expect(isDatasetExtraValid('[]')).toBe(false); +}); + +test('editing certification does not overwrite malformed Extra JSON', () => { + expect( + setDatasetCertification('{"custom_key":', { + certified_by: 'Data Platform Team', + }), + ).toBe('{"custom_key":'); +}); diff --git a/superset-frontend/src/components/Datasource/components/Fieldset/index.tsx b/superset-frontend/src/components/Datasource/components/Fieldset/index.tsx index e2f77be2c47e..849cbdb505f7 100644 --- a/superset-frontend/src/components/Datasource/components/Fieldset/index.tsx +++ b/superset-frontend/src/components/Datasource/components/Fieldset/index.tsx @@ -25,6 +25,7 @@ import Field from '../Field'; export interface FieldsetProps { children: ReactNode; onChange?: (updatedItem: Record) => void; + onFieldChange?: (fieldKey: fieldKeyType, value: unknown) => void; item?: Record; title?: ReactNode; compact?: boolean; @@ -36,6 +37,7 @@ type fieldKeyType = string | number; export default function Fieldset({ children, onChange, + onFieldChange, item = {}, title = null, compact = false, @@ -53,12 +55,21 @@ export default function Fieldset({ const handleChange = useCallback( (fieldKey: fieldKeyType, val: any) => { - onChange?.({ + const updatedItem = { ...itemRef.current, [fieldKey]: val, - }); + }; + // Multiple debounced controls can commit in the same React batch, before + // the effect above has synchronized the item passed back by the parent. + // Advance the ref synchronously so the later commit includes its sibling. + itemRef.current = updatedItem; + if (onFieldChange) { + onFieldChange(fieldKey, val); + } else { + onChange?.(updatedItem); + } }, - [onChange], + [onChange, onFieldChange], ); const propExtender = (field: { props: { fieldKey: fieldKeyType } }) => ({ diff --git a/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.test.tsx new file mode 100644 index 000000000000..c294dd2cdd2c --- /dev/null +++ b/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.test.tsx @@ -0,0 +1,45 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { render } from 'spec/helpers/testing-library'; + +import { BaseModalWrapper } from './SharedStyles'; + +test('filter config modal is bounded by its containing block', () => { + const { rerender } = render( + +
Modal content
+ , + ); + + const modal = document.querySelector('.ant-modal'); + expect(modal).toBeInTheDocument(); + expect(modal).toHaveStyleRule('width', '880px!important'); + expect(modal).toHaveStyleRule('max-width', 'calc(100% - 32px)'); + expect(modal).not.toHaveStyleRule('min-width', '880px'); + + rerender( + +
Modal content
+ , + ); + expect(document.querySelector('.ant-modal')).toHaveStyleRule( + 'width', + '100%!important', + ); +}); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx b/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx index 1d7ac1b67c6c..96d4c85a8ec9 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/ConfigModal/SharedStyles.tsx @@ -20,7 +20,7 @@ import { styled, css } from '@apache-superset/core/theme'; import { Form, StyledModal } from '@superset-ui/core/components'; const MODAL_MARGIN = 16; -const MIN_WIDTH = 880; +const MODAL_WIDTH = 880; export interface BaseModalWrapperProps { expanded: boolean; @@ -31,13 +31,9 @@ export interface BaseModalBodyProps { } export const BaseModalWrapper = styled(StyledModal)` - min-width: ${MIN_WIDTH}px; - width: ${({ expanded }) => (expanded ? '100%' : MIN_WIDTH)} !important; - - @media (max-width: ${MIN_WIDTH + MODAL_MARGIN * 2}px) { - width: 100% !important; - min-width: auto; - } + width: ${({ expanded }) => + expanded ? '100%' : `${MODAL_WIDTH}px`} !important; + max-width: calc(100% - ${MODAL_MARGIN * 2}px); .ant-modal-header { margin-bottom: 0; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DatasetSelect.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DatasetSelect.tsx index 71285fff8318..4a88350fcb5f 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DatasetSelect.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/DatasetSelect.tsx @@ -17,12 +17,10 @@ * under the License. */ import { useCallback, useMemo, ReactNode } from 'react'; -import rison from 'rison'; import { t } from '@apache-superset/core/translation'; import { isFeatureEnabled, FeatureFlag, - JsonResponse, ClientErrorObject, getClientErrorObject, } from '@superset-ui/core'; @@ -32,6 +30,7 @@ import { Dataset, DatasetSelectLabel, } from 'src/features/datasets/DatasetSelectLabel'; +import { fetchDatasourceList } from 'src/features/datasets/fetchDatasourceList'; import { datasetLabel, datasetLabelLower, @@ -95,27 +94,10 @@ export const loadDatasetOptions = async ( excludeDatasetIds: number[] = [], ) => { const useSemanticLayers = isFeatureEnabled(FeatureFlag.SemanticLayers); - const query = rison.encode({ - ...(useSemanticLayers - ? {} - : { - columns: ['id', 'table_name', 'database.database_name', 'schema'], - }), - filters: [{ col: 'table_name', opr: 'ct', value: search }], - page, - page_size: pageSize, - order_column: 'table_name', - order_direction: 'asc', - }); - const endpoint = useSemanticLayers - ? `/api/v1/datasource/?q=${query}` - : `/api/v1/dataset/?q=${query}`; - return cachedSupersetGet({ - endpoint, - }) - .then((response: JsonResponse) => { - const filteredResult = response.json.result.filter( - (item: Dataset) => !isExcludedDatasource(item, excludeDatasetIds), + return fetchDatasourceList(search, page, pageSize, { get: cachedSupersetGet }) + .then(({ result, count }) => { + const filteredResult = result.filter( + item => !isExcludedDatasource(item, excludeDatasetIds), ); const list: { @@ -123,7 +105,7 @@ export const loadDatasetOptions = async ( value: string | number; table_name: string; kind?: string; - }[] = filteredResult.map((item: Dataset) => ({ + }[] = filteredResult.map(item => ({ ...item, label: DatasetSelectLabel(item), value: useSemanticLayers @@ -134,7 +116,7 @@ export const loadDatasetOptions = async ( })); return { data: list, - totalCount: response.json.count ?? 0, + totalCount: count, }; }) .catch(async error => { diff --git a/superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx b/superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx index 153f91df6a51..bfd71472f6df 100644 --- a/superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx +++ b/superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx @@ -354,7 +354,6 @@ export default function DateFilterLabel(props: DateFilterControlProps) { const popoverContent = ( isOverflowingFilterBar diff --git a/superset-frontend/src/explore/components/controls/DateFilterControl/tests/DateFilterLabel.test.tsx b/superset-frontend/src/explore/components/controls/DateFilterControl/tests/DateFilterLabel.test.tsx index 455bb2d38a96..72dc40d9682e 100644 --- a/superset-frontend/src/explore/components/controls/DateFilterControl/tests/DateFilterLabel.test.tsx +++ b/superset-frontend/src/explore/components/controls/DateFilterControl/tests/DateFilterLabel.test.tsx @@ -29,6 +29,10 @@ import { } from 'spec/helpers/testing-library'; import { NO_TIME_RANGE, fetchTimeRange } from '@superset-ui/core'; +import { + PopoverProps, + SHIFT_INTO_VIEWPORT, +} from '../../ControlPopover/ControlPopover'; import DateFilterLabel from '..'; import { DateFilterControlProps } from '../types'; import { DateFilterTestKey } from '../utils'; @@ -46,6 +50,18 @@ const FIELD_TOOLTIP = '2024-01-01 ≤ col < 2024-01-08'; const DESCRIPTION_TOOLTIP = 'This control filters the whole chart based on the selected time range.'; +const mockPopoverProps: PopoverProps[] = []; +jest.mock('@superset-ui/core/components', () => { + const actual = jest.requireActual('@superset-ui/core/components'); + const Probe = (props: PopoverProps) => { + mockPopoverProps.push(props); + return ; + }; + return new Proxy(actual, { + get: (target, name) => (name === 'Popover' ? Probe : target[name]), + }); +}); + const mockStore = configureMockStore([thunk]); const defaultProps = { @@ -57,6 +73,7 @@ const defaultProps = { beforeEach(() => { mockedFetchTimeRange.mockReset(); mockedFetchTimeRange.mockResolvedValue({ value: FIELD_TOOLTIP }); + mockPopoverProps.length = 0; }); function setup( @@ -123,8 +140,27 @@ test('DateFilter popover should attach to document.body when not overflowing', ( userEvent.click(screen.getByText(NO_TIME_RANGE)); - const popover = document.querySelector('.time-range-popover'); + const popover = document.querySelector('.time-range-popover'); expect(popover?.parentElement).toBe(document.body); + expect(popover).toHaveStyle({ + width: 'min(600px, calc(100vw - 32px))', + }); +}); + +test('DateFilter popover shifts into the viewport', async () => { + render(setup()); + + userEvent.click(screen.getByText(NO_TIME_RANGE)); + + await waitFor(() => { + expect(mockPopoverProps).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + autoAdjustOverflow: SHIFT_INTO_VIEWPORT, + }), + ]), + ); + }); }); test('DateFilter popover should attach to parent node when overflowing in filter bar', () => { @@ -132,10 +168,13 @@ test('DateFilter popover should attach to parent node when overflowing in filter userEvent.click(screen.getByText(NO_TIME_RANGE)); - const popover = document.querySelector('.time-range-popover'); + const popover = document.querySelector('.time-range-popover'); const trigger = screen.getByTestId(DateFilterTestKey.PopoverOverlay); expect(popover?.parentElement).toBe(trigger.parentElement); + expect(popover).toHaveStyle({ + width: 'min(600px, calc(100vw - 32px))', + }); }); test('DateFilter should properly handle isOverflowingFilterBar prop changes', () => { diff --git a/superset-frontend/src/features/datasets/DatasetSelectLabel/index.tsx b/superset-frontend/src/features/datasets/DatasetSelectLabel/index.tsx index fa65f31e3b89..2611a923b97e 100644 --- a/superset-frontend/src/features/datasets/DatasetSelectLabel/index.tsx +++ b/superset-frontend/src/features/datasets/DatasetSelectLabel/index.tsx @@ -25,13 +25,18 @@ type Database = { database_name: string; }; +/** + * One row of a dataset or combined datasource listing. Semantic views report + * ``kind: 'semantic_view'`` and carry no schema; their ``database`` names the + * owning semantic layer. + */ export type Dataset = { id: number; table_name: string; datasource_type?: string; kind?: string; - schema: string; - database?: Database; + schema?: string | null; + database?: Database | null; }; const TooltipContent = styled.div` diff --git a/superset-frontend/src/features/datasets/fetchDatasourceList.test.ts b/superset-frontend/src/features/datasets/fetchDatasourceList.test.ts new file mode 100644 index 000000000000..cb4f51f64423 --- /dev/null +++ b/superset-frontend/src/features/datasets/fetchDatasourceList.test.ts @@ -0,0 +1,75 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import fetchMock from 'fetch-mock'; +import { isFeatureEnabled } from '@superset-ui/core'; +import { fetchDatasourceList } from './fetchDatasourceList'; + +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: jest.fn(() => false), +})); + +const mockIsFeatureEnabled = jest.mocked(isFeatureEnabled); + +const emptyPage = { result: [], count: 0 }; + +beforeEach(() => { + fetchMock.removeRoutes(); + fetchMock.clearHistory(); + fetchMock.get('glob:*/api/v1/dataset/*', emptyPage); + fetchMock.get('glob:*/api/v1/datasource/*', emptyPage); + mockIsFeatureEnabled.mockReturnValue(false); +}); + +const requestedUrls = () => + fetchMock.callHistory.calls().map(call => decodeURIComponent(call.url)); + +test('uses the combined endpoint when the flag is on and the dataset endpoint when off', async () => { + mockIsFeatureEnabled.mockReturnValue(true); + await fetchDatasourceList('orders', 0, 25); + mockIsFeatureEnabled.mockReturnValue(false); + await fetchDatasourceList('orders', 0, 25); + + const urls = requestedUrls(); + expect(urls[0]).toContain('/api/v1/datasource/'); + expect(urls[1]).toContain('/api/v1/dataset/'); +}); + +test('exactMatch with datasetsOnly stays on the dataset endpoint with an eq filter', async () => { + mockIsFeatureEnabled.mockReturnValue(true); + await fetchDatasourceList('orders', 0, 1, { + exactMatch: true, + datasetsOnly: true, + }); + + const urls = requestedUrls(); + expect(urls[0]).toContain('/api/v1/dataset/'); + expect(urls[0]).toContain('opr:eq'); +}); + +test('exactMatch without datasetsOnly is refused, not silently unfiltered', async () => { + // The combined endpoint's filter parser honours only substring (ct) name + // filters; an eq filter is dropped and the page comes back unfiltered. + mockIsFeatureEnabled.mockReturnValue(true); + expect(() => + // @ts-expect-error deliberately violating the option coupling + fetchDatasourceList('orders', 0, 1, { exactMatch: true }), + ).toThrow(/exactMatch requires datasetsOnly/); + expect(requestedUrls()).toHaveLength(0); +}); diff --git a/superset-frontend/src/features/datasets/fetchDatasourceList.ts b/superset-frontend/src/features/datasets/fetchDatasourceList.ts new file mode 100644 index 000000000000..358df7065a6a --- /dev/null +++ b/superset-frontend/src/features/datasets/fetchDatasourceList.ts @@ -0,0 +1,113 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import rison from 'rison'; +import { + FeatureFlag, + isFeatureEnabled, + JsonResponse, + SupersetClient, +} from '@superset-ui/core'; +import { Dataset } from './DatasetSelectLabel'; + +/** Columns requested from the dataset-only endpoint. */ +const DATASET_LIST_COLUMNS = [ + 'id', + 'table_name', + 'datasource_type', + 'database.database_name', + 'schema', +]; + +export type DatasourceListPage = { + result: Dataset[]; + count: number; +}; + +export type FetchDatasourceListOptions = { + /** Transport to use; pass a cached variant to share responses. */ + get?: typeof SupersetClient.get; +} & ( + | { + /** + * Match `search` against the name exactly rather than as a substring. + * Only supported together with `datasetsOnly`: the combined datasource + * endpoint's filter parser honours only substring (`ct`) name filters + * and silently drops an `eq` filter, so an exact-match preload against + * it would resolve to an arbitrary unfiltered page. + */ + exactMatch: true; + datasetsOnly: true; + } + | { + exactMatch?: false; + /** + * Query the dataset-only endpoint even when semantic layers are + * enabled. Used by callers that resolve a dataset by name and must not + * be answered with a same-named semantic view. + */ + datasetsOnly?: boolean; + } +); + +/** + * Fetches one page of datasources by name, ordered by name. + * + * When the SEMANTIC_LAYERS feature flag is enabled the combined datasource + * endpoint is queried and the page mixes datasets with semantic views + * (distinguished by `kind`); otherwise only datasets are listed. Every picker + * that offers datasources should load through here so the two endpoints stay + * behind one contract. + */ +export const fetchDatasourceList = ( + search: string, + page: number, + pageSize: number, + { + exactMatch = false, + datasetsOnly = false, + get = SupersetClient.get, + }: FetchDatasourceListOptions = {}, +): Promise => { + if (exactMatch && !datasetsOnly) { + // Enforced at the type level too; this guards plain-JS callers. + throw new Error( + 'fetchDatasourceList: exactMatch requires datasetsOnly — the combined ' + + 'datasource endpoint only supports substring name filters.', + ); + } + const useCombinedList = + !datasetsOnly && isFeatureEnabled(FeatureFlag.SemanticLayers); + const query = rison.encode({ + ...(useCombinedList ? {} : { columns: DATASET_LIST_COLUMNS }), + filters: [ + { col: 'table_name', opr: exactMatch ? 'eq' : 'ct', value: search }, + ], + page, + page_size: pageSize, + order_column: 'table_name', + order_direction: 'asc', + }); + const endpoint = useCombinedList + ? `/api/v1/datasource/?q=${query}` + : `/api/v1/dataset/?q=${query}`; + return get({ endpoint }).then((response: JsonResponse) => ({ + result: response.json.result as Dataset[], + count: response.json.count ?? 0, + })); +}; diff --git a/superset-frontend/src/features/datasets/types.ts b/superset-frontend/src/features/datasets/types.ts index 026372c39768..b182c3f78dd7 100644 --- a/superset-frontend/src/features/datasets/types.ts +++ b/superset-frontend/src/features/datasets/types.ts @@ -75,6 +75,9 @@ export type DatasetObject = { columns: ColumnObject[]; metrics: MetricObject[]; extra?: string; + certified_by?: string; + certification_details?: string; + dataset_certification_changed?: boolean; is_managed_externally: boolean; normalize_columns: boolean; always_filter_main_dttm: boolean; diff --git a/superset-frontend/src/pages/ArchivedList/ArchivedDatasetPurgeModal.tsx b/superset-frontend/src/pages/ArchivedList/ArchivedDatasetPurgeModal.tsx new file mode 100644 index 000000000000..4d93b8c19ad4 --- /dev/null +++ b/superset-frontend/src/pages/ArchivedList/ArchivedDatasetPurgeModal.tsx @@ -0,0 +1,216 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { useState } from 'react'; +import { t } from '@apache-superset/core/translation'; +import { styled } from '@apache-superset/core/theme'; +import { Button, DeleteModal } from '@superset-ui/core/components'; +import type { + ArchivedDatasetPurgeModalState, + PurgeImpactCollection, + PurgeImpactItem, +} from './types'; + +const ImpactSection = styled.section` + ${({ theme }) => ` + margin-top: ${theme.sizeUnit * 4}px; + `} +`; + +const ImpactHeading = styled.h4` + margin-bottom: ${({ theme }) => theme.sizeUnit * 2}px; +`; + +const ImpactList = styled.ul` + margin-bottom: ${({ theme }) => theme.sizeUnit * 2}px; + padding-left: ${({ theme }) => theme.sizeUnit * 5}px; +`; + +const ArchivedMarker = styled.span` + ${({ theme }) => ` + color: ${theme.colorTextSecondary}; + margin-left: ${theme.sizeUnit}px; + `} +`; + +const Message = styled.p` + margin-top: ${({ theme }) => theme.sizeUnit * 3}px; +`; + +const DEFAULT_VISIBLE_ITEMS = 5; + +function ImpactItem({ item }: { item: PurgeImpactItem }) { + // Only link server-issued application paths; anything else (absolute or + // scheme-relative URLs, javascript: and data: schemes) renders as text. + const isSafeAppPath = Boolean( + item.url && item.url.startsWith('/') && !item.url.startsWith('//'), + ); + const label = + item.url && isSafeAppPath && !item.archived ? ( + {item.name} + ) : ( + item.name + ); + + return ( +
  • + {label} + {item.archived && ( + + {t('(archived)')} + + )} +
    • + ); +} + +function ImpactCollection({ + collection, + singular, + plural, +}: { + collection: PurgeImpactCollection; + singular: string; + plural: string; +}) { + const [expanded, setExpanded] = useState(false); + const visibleItems = expanded + ? collection.result + : collection.result.slice(0, DEFAULT_VISIBLE_ITEMS); + const hasMore = collection.result.length > DEFAULT_VISIBLE_ITEMS; + + return ( + + + {t('%(count)s %(label)s', { + count: collection.count, + label: collection.count === 1 ? singular : plural, + })} + + {collection.count === 0 ? ( +

    {t('No affected %(label)s.', { label: plural.toLowerCase() })}

    + ) : ( + <> + {visibleItems.length > 0 && ( + + {visibleItems.map(item => ( + + ))} + + )} + {collection.restricted_count > 0 && ( +

    + {t('%(count)s additional restricted %(label)s', { + count: collection.restricted_count, + label: collection.restricted_count === 1 ? singular : plural, + })} +

    + )} + {hasMore && ( + + )} + + )} +     + ); +} + +export interface ArchivedDatasetPurgeModalProps { + state: Exclude; + onConfirm: () => void; + onHide: () => void; + onRetry: () => void; +} + +export function ArchivedDatasetPurgeModal({ + state, + onConfirm, + onHide, + onRetry, +}: ArchivedDatasetPurgeModalProps) { + const name = String(state.item.table_name ?? ''); + const hasImpact = + state.status === 'ready' || + state.status === 'submitting' || + state.status === 'changed'; + const impact = hasImpact ? state.impact : undefined; + const unavailable = state.status === 'error'; + + return ( + + {state.status === 'loading' && ( +

    {t('Checking charts and dashboards that use this dataset…')}

    + )} + {state.status === 'changed' && ( + + {t( + 'The affected charts or dashboards changed. Review the updated impact and type DELETE again to continue.', + )} + + )} + {unavailable && ( + <> + + {t( + 'The deletion impact could not be determined. This dataset cannot be permanently deleted until the check succeeds.', + )} + + + + )} + {impact && ( + <> +

    + {t( + 'Deleting this dataset is permanent. The affected charts and dashboards will remain, but they may no longer work.', + )} +

    + + + + )} + + } + disablePrimaryButton={state.status === 'loading' || unavailable} + loading={state.status === 'submitting'} + confirmationResetKey={ + impact ? `${state.status}:${impact.impact_token}` : state.status + } + onConfirm={onConfirm} + onHide={onHide} + /> + ); +} diff --git a/superset-frontend/src/pages/ArchivedList/ArchivedList.test.tsx b/superset-frontend/src/pages/ArchivedList/ArchivedList.test.tsx index 849e3164b9bc..4c77bfd34e55 100644 --- a/superset-frontend/src/pages/ArchivedList/ArchivedList.test.tsx +++ b/superset-frontend/src/pages/ArchivedList/ArchivedList.test.tsx @@ -56,6 +56,43 @@ const dashboardInfoEndpoint = 'glob:*/api/v1/dashboard/_info*'; const dashboardListEndpoint = 'glob:*/api/v1/dashboard/?*'; const datasetInfoEndpoint = 'glob:*/api/v1/dataset/_info*'; const datasetListEndpoint = 'glob:*/api/v1/dataset/?*'; +const datasetImpactEndpoint = 'glob:*/api/v1/dataset/*/purge-impact'; +const datasetPurgeEndpoint = 'glob:*/api/v1/dataset/*/purge'; + +const buildImpact = (overrides: Record = {}) => ({ + impact_token: 'v1:reviewed-impact', + charts: { count: 0, restricted_count: 0, result: [] }, + dashboards: { count: 0, restricted_count: 0, result: [] }, + ...overrides, +}); + +const buildPositiveImpact = () => + buildImpact({ + charts: { + count: 2, + restricted_count: 1, + result: [ + { + uuid: 'accessible-chart', + name: 'Revenue chart', + archived: true, + url: null, + }, + ], + }, + dashboards: { + count: 1, + restricted_count: 0, + result: [ + { + uuid: 'accessible-dashboard', + name: 'Executive dashboard', + archived: false, + url: '/superset/dashboard/accessible-dashboard/', + }, + ], + }, + }); const mockDashboards = [ { id: 10, uuid: 'dash-uuid-1', dashboard_title: 'Deleted Dashboard One' }, @@ -100,6 +137,7 @@ jest.mock('@superset-ui/core', () => ({ })); const mockAddDangerToast = jest.fn(); +const mockAddSuccessToast = jest.fn(); jest.mock('src/components/MessageToasts/withToasts', () => ({ __esModule: true, default: @@ -108,7 +146,7 @@ jest.mock('src/components/MessageToasts/withToasts', () => ({ @@ -118,6 +156,7 @@ jest.mock('src/components/MessageToasts/withToasts', () => ({ const mockRoutes = ( restoreStatus = 200, purgeResponse: Parameters[1] = {}, + impactResponse: Parameters[1] = buildPositiveImpact(), ) => { fetchMock.get(infoEndpoint, { permissions: ['can_read', 'can_write'] }); fetchMock.get(listEndpoint, { result: mockCharts, count: mockCharts.length }); @@ -137,6 +176,8 @@ const mockRoutes = ( result: mockDatasets, count: mockDatasets.length, }); + fetchMock.get(datasetImpactEndpoint, impactResponse); + fetchMock.post(datasetPurgeEndpoint, purgeResponse); }; const renderArchivedList = (withStore = store) => @@ -153,6 +194,7 @@ beforeEach(() => { fetchMock.removeRoutes(); fetchMock.clearHistory(); mockAddDangerToast.mockClear(); + mockAddSuccessToast.mockClear(); }); afterEach(() => { @@ -645,3 +687,159 @@ test('labels the dataset type "Dataset" when semantic layers is disabled', async within(datasetRow as HTMLElement).getByText('Dataset'), ).toBeInTheDocument(); }); + +const openDatasetPurgeModal = async () => { + await selectOption('Dataset', 'Type'); + await screen.findByText('deleted_table_one'); + fireEvent.click((await screen.findAllByTestId('archived-row-purge'))[0]); +}; + +test('dataset purge shows positive, archived, and restricted impact', async () => { + mockRoutes(); + renderArchivedList(); + await screen.findByTestId('archived-list-view'); + + await openDatasetPurgeModal(); + + expect(await screen.findByText('2 Charts')).toBeInTheDocument(); + expect(screen.getByText('Revenue chart')).toBeInTheDocument(); + expect( + within(screen.getByRole('dialog')).getByLabelText('Archived'), + ).toBeInTheDocument(); + expect(screen.getByText('1 additional restricted Chart')).toBeInTheDocument(); + expect(screen.getByText('Executive dashboard').closest('a')).not.toBeNull(); +}); + +test('dataset purge renders an explicit zero only after impact loads', async () => { + mockRoutes(200, {}, buildImpact()); + renderArchivedList(); + await screen.findByTestId('archived-list-view'); + + await openDatasetPurgeModal(); + + expect(await screen.findByText('No affected charts.')).toBeInTheDocument(); + expect(screen.getByText('No affected dashboards.')).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Delete' })).toBeDisabled(); + await userEvent.type(screen.getByTestId('delete-modal-input'), 'DELETE'); + expect(screen.getByRole('button', { name: 'Delete' })).toBeEnabled(); +}); + +test('dataset purge fails closed when impact is unavailable', async () => { + mockRoutes(200, {}, 500); + renderArchivedList(); + await screen.findByTestId('archived-list-view'); + + await openDatasetPurgeModal(); + + expect( + await screen.findByText(/deletion impact could not be determined/i), + ).toBeInTheDocument(); + expect(screen.getByRole('button', { name: 'Retry' })).toBeEnabled(); + expect(screen.getByRole('button', { name: 'Delete' })).toBeDisabled(); + expect(screen.queryByText('No affected charts.')).not.toBeInTheDocument(); + expect(fetchMock.callHistory.calls(datasetPurgeEndpoint)).toHaveLength(0); +}); + +test('a purge that succeeds after the modal closes still toasts and refetches', async () => { + let resolvePurge: (value: { + status: number; + body: object; + }) => void = () => {}; + const deferredPurge = new Promise<{ status: number; body: object }>( + resolve => { + resolvePurge = resolve; + }, + ); + mockRoutes(200, deferredPurge); + renderArchivedList(); + await screen.findByTestId('archived-list-view'); + await openDatasetPurgeModal(); + await screen.findByText('2 Charts'); + + await userEvent.type(screen.getByTestId('delete-modal-input'), 'DELETE'); + await userEvent.click(screen.getByRole('button', { name: 'Delete' })); + const listCallsBefore = + fetchMock.callHistory.calls(datasetListEndpoint).length; + + // The user closes the modal while the purge request is still in flight; + // the deletion still happens server-side. + await userEvent.click(screen.getByRole('button', { name: 'Cancel' })); + expect(screen.queryByRole('dialog')).not.toBeInTheDocument(); + + resolvePurge({ status: 200, body: { message: 'OK' } }); + + await waitFor(() => + expect(mockAddSuccessToast).toHaveBeenCalledWith( + expect.stringContaining('deleted successfully'), + ), + ); + await waitFor(() => + expect( + fetchMock.callHistory.calls(datasetListEndpoint).length, + ).toBeGreaterThan(listCallsBefore), + ); +}); + +test('impact entries with non-application URLs render as text, not links', async () => { + mockRoutes( + 200, + {}, + buildImpact({ + dashboards: { + count: 2, + restricted_count: 0, + result: [ + { + uuid: 'external-dashboard', + name: 'External dashboard', + archived: false, + url: 'https://evil.example/dashboard', + }, + { + uuid: 'schemeless-dashboard', + name: 'Schemeless dashboard', + archived: false, + url: '//evil.example/dashboard', + }, + ], + }, + }), + ); + renderArchivedList(); + await screen.findByTestId('archived-list-view'); + + await openDatasetPurgeModal(); + + expect(await screen.findByText('External dashboard')).toBeInTheDocument(); + expect(screen.getByText('External dashboard').closest('a')).toBeNull(); + expect(screen.getByText('Schemeless dashboard').closest('a')).toBeNull(); +}); + +test('a 409 replaces impact and re-arms DELETE confirmation', async () => { + const changedImpact = buildImpact({ + impact_token: 'v1:changed-impact', + charts: { count: 1, restricted_count: 0, result: [] }, + }); + mockRoutes(200, { + status: 409, + body: { + message: 'Impact changed', + reason: 'purge_impact_changed', + impact: changedImpact, + }, + }); + renderArchivedList(); + await screen.findByTestId('archived-list-view'); + await openDatasetPurgeModal(); + await screen.findByText('2 Charts'); + + await userEvent.type(screen.getByTestId('delete-modal-input'), 'DELETE'); + await userEvent.click(screen.getByRole('button', { name: 'Delete' })); + + expect(await screen.findByRole('alert')).toHaveTextContent( + /affected charts or dashboards changed/i, + ); + expect(screen.getByText('1 Chart')).toBeInTheDocument(); + expect(screen.getByTestId('delete-modal-input')).toHaveValue(''); + expect(screen.getByRole('button', { name: 'Delete' })).toBeDisabled(); +}); diff --git a/superset-frontend/src/pages/ArchivedList/index.tsx b/superset-frontend/src/pages/ArchivedList/index.tsx index c6d5f811c5dd..4b2e109d1fd1 100644 --- a/superset-frontend/src/pages/ArchivedList/index.tsx +++ b/superset-frontend/src/pages/ArchivedList/index.tsx @@ -46,8 +46,12 @@ import { ARCHIVED_TYPES, ARCHIVED_TYPE_CONFIG, type ArchivedItem, + type ArchivedDatasetPurgeModalState, type ArchivedType, + type PurgeImpactChangedResponse, + type PurgeImpactResponse, } from './types'; +import { ArchivedDatasetPurgeModal } from './ArchivedDatasetPurgeModal'; /** Cell props shape shared by the column renderers below. */ type ArchivedCell = { row: { original: ArchivedItem } }; @@ -102,15 +106,29 @@ function ArchivedRowActions({ name, onRestore, onPurge, + previewBeforePurge = false, busy = false, }: { item: ArchivedItem; name: string; onRestore: (item: ArchivedItem) => void; onPurge: (item: ArchivedItem) => void; + previewBeforePurge?: boolean; /** A request for this row is in flight; both actions stand down. */ busy?: boolean; }) { + const permanentDeleteButton = (onClick: () => void) => ( + } + dataTest="archived-row-purge" + disabled={busy} + onClick={onClick} + /> + ); + return ( onRestore(item)} /> - onPurge(item)} - > - {confirmDelete => ( - } - dataTest="archived-row-purge" - disabled={busy} - onClick={confirmDelete} - /> - )} - + {previewBeforePurge ? ( + permanentDeleteButton(() => onPurge(item)) + ) : ( + onPurge(item)} + > + {confirmDelete => permanentDeleteButton(confirmDelete)} + + )} ); } @@ -188,6 +200,9 @@ function ArchivedListBody({ // so the buttons can render disabled meanwhile. const inFlightRef = useRef>(new Set()); const [inFlight, setInFlight] = useState([]); + const [datasetPurgeModal, setDatasetPurgeModal] = + useState({ status: 'closed' }); + const impactRequestGeneration = useRef(0); const beginAction = useCallback((uuid: string): boolean => { if (inFlightRef.current.has(uuid)) { @@ -286,6 +301,125 @@ function ArchivedListBody({ [performRowAction, addSuccessToast], ); + const loadDatasetPurgeImpact = useCallback(async (item: ArchivedItem) => { + const generation = impactRequestGeneration.current + 1; + impactRequestGeneration.current = generation; + setDatasetPurgeModal({ status: 'loading', item }); + + try { + const { json } = await SupersetClient.get({ + endpoint: `/api/v1/dataset/${item.uuid}/purge-impact`, + }); + if (impactRequestGeneration.current !== generation) { + return; + } + setDatasetPurgeModal({ + status: 'ready', + item, + impact: json as PurgeImpactResponse, + }); + } catch (error) { + if (impactRequestGeneration.current !== generation) { + return; + } + const { error: message } = await getClientErrorObject(error); + if (impactRequestGeneration.current === generation) { + setDatasetPurgeModal({ + status: 'error', + item, + message, + }); + } + } + }, []); + + const closeDatasetPurgeModal = useCallback(() => { + impactRequestGeneration.current += 1; + setDatasetPurgeModal({ status: 'closed' }); + }, []); + + const retryDatasetPurgeImpact = useCallback(() => { + if (datasetPurgeModal.status !== 'closed') { + loadDatasetPurgeImpact(datasetPurgeModal.item); + } + }, [datasetPurgeModal, loadDatasetPurgeImpact]); + + const confirmDatasetPurge = useCallback(async () => { + if ( + datasetPurgeModal.status !== 'ready' && + datasetPurgeModal.status !== 'changed' + ) { + return; + } + + const { item, impact } = datasetPurgeModal; + if (!beginAction(item.uuid)) { + return; + } + const generation = impactRequestGeneration.current + 1; + impactRequestGeneration.current = generation; + setDatasetPurgeModal({ status: 'submitting', item, impact }); + + try { + await SupersetClient.post({ + endpoint: `/api/v1/dataset/${item.uuid}/purge`, + body: JSON.stringify({ + confirmed_impact_token: impact.impact_token, + }), + headers: { 'Content-Type': 'application/json' }, + }); + // The purge succeeded even if the modal was closed while the request + // was in flight, so the toast and refresh must not be gated on the + // request generation — only the modal state is. + const name = String(item[config.nameField] ?? ''); + addSuccessToast(t('%(name)s deleted successfully', { name })); + if (impactRequestGeneration.current === generation) { + setDatasetPurgeModal({ status: 'closed' }); + } + await refreshData(); + } catch (error) { + if (impactRequestGeneration.current !== generation) { + return; + } + const parsedError = (await getClientErrorObject(error)) as Awaited< + ReturnType + > & + Partial & { status?: number }; + if ( + parsedError.status === 409 && + parsedError.reason === 'purge_impact_changed' && + parsedError.impact + ) { + setDatasetPurgeModal({ + status: 'changed', + item, + impact: parsedError.impact, + message: parsedError.message ?? parsedError.error, + }); + } else if (parsedError.status === 404) { + setDatasetPurgeModal({ status: 'closed' }); + addDangerToast(parsedError.error); + await refreshData(); + } else { + setDatasetPurgeModal({ + status: 'error', + item, + message: parsedError.error, + }); + } + } finally { + endAction(item.uuid); + } + }, [ + datasetPurgeModal, + beginAction, + endAction, + config.nameField, + addSuccessToast, + addDangerToast, + refreshData, + ]); + const columns = useMemo( () => [ { @@ -349,7 +483,8 @@ function ArchivedListBody({ item={original} name={String(original[config.nameField] ?? '')} onRestore={handleRestore} - onPurge={handlePurge} + onPurge={type === 'dataset' ? loadDatasetPurgeImpact : handlePurge} + previewBeforePurge={type === 'dataset'} busy={inFlight.includes(original.uuid)} /> ), @@ -359,7 +494,14 @@ function ArchivedListBody({ size: 'sm', }, ], - [config.nameField, type, handleRestore, handlePurge, inFlight], + [ + config.nameField, + type, + handleRestore, + handlePurge, + loadDatasetPurgeImpact, + inFlight, + ], ); // Default to most-recently-archived first. `deleted_at` is orderable on all @@ -421,24 +563,34 @@ function ArchivedListBody({ ); return ( - - className="archived-list-view" - columns={columns} - filters={filters} - data={resourceCollection} - count={resourceCount} - pageSize={PAGE_SIZE} - fetchData={fetchData} - refreshData={refreshData} - addSuccessToast={addSuccessToast} - addDangerToast={addDangerToast} - loading={loading} - initialSort={initialSort} - emptyState={{ - title: t('No archived items'), - image: 'empty.svg', - }} - /> + <> + + className="archived-list-view" + columns={columns} + filters={filters} + data={resourceCollection} + count={resourceCount} + pageSize={PAGE_SIZE} + fetchData={fetchData} + refreshData={refreshData} + addSuccessToast={addSuccessToast} + addDangerToast={addDangerToast} + loading={loading} + initialSort={initialSort} + emptyState={{ + title: t('No archived items'), + image: 'empty.svg', + }} + /> + {datasetPurgeModal.status !== 'closed' && ( + + )} + ); } diff --git a/superset-frontend/src/pages/ArchivedList/types.ts b/superset-frontend/src/pages/ArchivedList/types.ts index 39a33537c207..c486666c6250 100644 --- a/superset-frontend/src/pages/ArchivedList/types.ts +++ b/superset-frontend/src/pages/ArchivedList/types.ts @@ -103,3 +103,41 @@ export interface ArchivedItem { // dynamically via the type config, so the index signature remains. [key: string]: unknown; } + +export interface PurgeImpactItem { + uuid: string; + name: string; + archived: boolean; + url?: string | null; +} + +export interface PurgeImpactCollection { + count: number; + restricted_count: number; + result: PurgeImpactItem[]; +} + +export interface PurgeImpactResponse { + impact_token: string; + charts: PurgeImpactCollection; + dashboards: PurgeImpactCollection; +} + +export interface PurgeImpactChangedResponse { + message: string; + reason: 'purge_impact_changed'; + impact: PurgeImpactResponse; +} + +export type ArchivedDatasetPurgeModalState = + | { status: 'closed' } + | { status: 'loading'; item: ArchivedItem } + | { status: 'ready'; item: ArchivedItem; impact: PurgeImpactResponse } + | { status: 'submitting'; item: ArchivedItem; impact: PurgeImpactResponse } + | { + status: 'changed'; + item: ArchivedItem; + impact: PurgeImpactResponse; + message: string; + } + | { status: 'error'; item: ArchivedItem; message: string }; diff --git a/superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx b/superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx index 21b439ba84d8..5ef9e07d7542 100644 --- a/superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx +++ b/superset-frontend/src/pages/ChartCreation/ChartCreation.test.tsx @@ -24,6 +24,7 @@ import { waitFor, } from 'spec/helpers/testing-library'; import fetchMock from 'fetch-mock'; +import { FeatureFlag, isFeatureEnabled } from '@superset-ui/core'; import { ChartCreation } from 'src/pages/ChartCreation'; import { UserWithPermissionsAndRoles } from 'src/types/bootstrapTypes'; @@ -33,6 +34,18 @@ jest.mock('src/components/DynamicPlugins', () => ({ }), })); +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + isFeatureEnabled: jest.fn(), +})); + +const mockIsFeatureEnabled = jest.mocked(isFeatureEnabled); + +const enableSemanticLayers = () => + mockIsFeatureEnabled.mockImplementation( + flag => flag === FeatureFlag.SemanticLayers, + ); + const mockDatasourceResponse = { result: [ { @@ -46,9 +59,42 @@ const mockDatasourceResponse = { count: 1, }; -fetchMock.get(/\/api\/v1\/dataset\/\?q=.*/, { - body: mockDatasourceResponse, - status: 200, +const legacyDatasetFixtures = [ + { + id: 42, + table_name: 'shared_source', + datasource_type: 'table', + database: { database_name: 'examples' }, + schema: 'public', + }, +]; + +const combinedDatasourceFixtures = [ + { + id: 42, + table_name: 'shared_source', + kind: 'physical', + source_type: 'database', + database: { database_name: 'examples' }, + schema: 'public', + }, + { + id: 42, + table_name: 'shared_source', + kind: 'semantic_view', + source_type: 'semantic_layer', + database: { database_name: 'Sales semantics' }, + schema: null, + }, +]; + +beforeEach(() => { + mockIsFeatureEnabled.mockReturnValue(false); + fetchMock.clearHistory().removeRoutes(); + fetchMock.get(/\/api\/v1\/dataset\/\?q=.*/, { + body: mockDatasourceResponse, + status: 200, + }); }); const mockUser: UserWithPermissionsAndRoles = { @@ -448,3 +494,285 @@ test('shows only exact match when loading dataset from URL, not partial matches' locationSpy.mockRestore(); }); + +test('lists a same-named dataset and semantic view as distinct, typed options', async () => { + enableSemanticLayers(); + fetchMock.get(/\/api\/v1\/datasource\/\?q=.*/, { + body: { result: combinedDatasourceFixtures, count: 2 }, + status: 200, + }); + + await renderComponent(); + userEvent.click(screen.getByRole('combobox', { name: 'Datasource' })); + + expect(await screen.findAllByText('shared_source')).toHaveLength(2); + // The type label is plain text inside each option rather than colour or an + // icon, so it is read out along with the name. + const optionTexts = Array.from( + document.querySelectorAll('.ant-select-item-option-content'), + option => option.textContent, + ); + expect(optionTexts).toEqual([ + expect.stringMatching(/^shared_source.*Dataset$/), + expect.stringMatching(/^shared_source.*Semantic View$/), + ]); + expect(screen.getByText('Dataset')).not.toHaveAttribute( + 'aria-hidden', + 'true', + ); + expect(screen.getByText('Semantic View')).not.toHaveAttribute( + 'aria-hidden', + 'true', + ); +}); + +test('requests unified server ordering by table name', async () => { + enableSemanticLayers(); + fetchMock.get(/\/api\/v1\/datasource\/\?q=.*/, { + body: { + result: [ + { + id: 7, + table_name: 'alpha_semantic_view', + kind: 'semantic_view', + source_type: 'semantic_layer', + database: { database_name: 'Sales semantics' }, + schema: null, + }, + { + id: 8, + table_name: 'beta_dataset', + kind: 'physical', + source_type: 'database', + database: { database_name: 'examples' }, + schema: 'public', + }, + ], + count: 2, + }, + status: 200, + }); + + await renderComponent(); + userEvent.click(screen.getByRole('combobox', { name: 'Datasource' })); + + await screen.findByText('alpha_semantic_view'); + expect( + fetchMock.callHistory.calls().some(call => { + const decodedUrl = decodeURIComponent(call.url); + return ( + decodedUrl.includes('order_column:table_name') && + decodedUrl.includes('order_direction:asc') + ); + }), + ).toBe(true); +}); + +test('searches semantic views through the combined datasource endpoint', async () => { + enableSemanticLayers(); + fetchMock.get(/\/api\/v1\/datasource\/\?q=.*/, { + // More results than are loaded, so searching goes back to the server. + body: { result: [combinedDatasourceFixtures[1]], count: 26 }, + status: 200, + }); + + await renderComponent(); + const datasourceSelect = screen.getByRole('combobox', { + name: 'Datasource', + }); + userEvent.click(datasourceSelect); + userEvent.type(datasourceSelect, 'shared'); + + expect(await screen.findByText('shared_source')).toBeInTheDocument(); + await waitFor(() => + expect( + fetchMock.callHistory.calls().some(call => { + const decodedUrl = decodeURIComponent(call.url); + return ( + decodedUrl.includes('/api/v1/datasource/') && + decodedUrl.includes('col:table_name') && + decodedUrl.includes('opr:ct') && + decodedUrl.includes('shared') + ); + }), + ).toBe(true), + ); +}); + +test('navigates to Explore with the semantic view composite identity', async () => { + enableSemanticLayers(); + fetchMock.get(/\/api\/v1\/datasource\/\?q=.*/, { + body: { result: [combinedDatasourceFixtures[1]], count: 1 }, + status: 200, + }); + + await renderComponent(); + userEvent.click(screen.getByRole('combobox', { name: 'Datasource' })); + userEvent.click(await screen.findByText('shared_source')); + userEvent.click(screen.getByRole('tab', { name: /All charts/i })); + userEvent.dblClick(await screen.findByText('Table')); + + expect(mockHistoryPush).toHaveBeenCalledWith( + '/explore/?viz_type=table&datasource=42__semantic_view', + ); +}); + +test('shows a failed datasource load as an error, then recovers on the next search', async () => { + enableSemanticLayers(); + const retryView = { + id: 43, + table_name: 'retry_view', + kind: 'semantic_view', + source_type: 'semantic_layer', + database: { database_name: 'Sales semantics' }, + schema: null, + }; + fetchMock.get(/\/api\/v1\/datasource\/\?q=.*/, ({ url }) => { + const request = decodeURIComponent(url); + if (request.includes('value:fail')) { + // An HTTP failure rather than a thrown error: the client retries + // network errors, which would only slow the test down. + return { status: 500, body: { message: 'datasource load failed' } }; + } + return { + body: { + result: request.includes('value:retry') + ? [retryView] + : [combinedDatasourceFixtures[1]], + count: 26, + }, + status: 200, + }; + }); + + await renderComponent(); + const datasourceSelect = screen.getByRole('combobox', { + name: 'Datasource', + }); + userEvent.click(datasourceSelect); + userEvent.click(await screen.findByText('shared_source')); + userEvent.click(datasourceSelect); + userEvent.type(datasourceSelect, 'fail'); + + // The failure is reported as such, not disguised as an empty result. + expect( + await screen.findByText('datasource load failed', {}, { timeout: 3000 }), + ).toBeInTheDocument(); + expect( + screen.queryByText('No data', { selector: '.ant-empty-description' }), + ).toBeNull(); + + // The next search replaces the error with its results. + userEvent.clear(datasourceSelect); + userEvent.type(datasourceSelect, 'retry'); + expect( + await screen.findByText('retry_view', {}, { timeout: 3000 }), + ).toBeInTheDocument(); + expect(screen.queryByText('datasource load failed')).not.toBeInTheDocument(); + + // The selection committed before the failure is still what gets created. + userEvent.click(screen.getByRole('tab', { name: /All charts/i })); + userEvent.dblClick(await screen.findByText('Table')); + expect(mockHistoryPush).toHaveBeenCalledWith( + '/explore/?viz_type=table&datasource=42__semantic_view', + ); +}); + +test('uses generic picker terminology without changing the dataset action', async () => { + enableSemanticLayers(); + fetchMock.get(/\/api\/v1\/datasource\/\?q=.*/, { + body: { result: combinedDatasourceFixtures, count: 2 }, + status: 200, + }); + + await renderComponent(mockUserWithDatasetWrite); + + expect( + screen.getByRole('combobox', { name: 'Datasource' }), + ).toBeInTheDocument(); + expect(screen.getAllByText('Choose a datasource')).toHaveLength(2); + const addDatasetLink = screen.getByRole('link', { name: 'Add a dataset' }); + expect(addDatasetLink).toHaveAttribute('href', '/dataset/add/'); +}); + +test('keeps the legacy dataset-only picker when semantic layers are disabled', async () => { + fetchMock.clearHistory().removeRoutes(); + fetchMock.get(/\/api\/v1\/dataset\/\?q=.*/, { + body: { result: legacyDatasetFixtures, count: 1 }, + status: 200, + }); + + await renderComponent(); + const datasourceSelect = screen.getByRole('combobox', { name: 'Dataset' }); + userEvent.click(datasourceSelect); + userEvent.click(await screen.findByText('shared_source')); + + expect(screen.queryByText('Semantic View')).not.toBeInTheDocument(); + expect(screen.queryByText('Dataset', { selector: '.ant-tag' })).toBeNull(); + expect( + fetchMock.callHistory + .calls() + .some(call => call.url.includes('/api/v1/datasource/')), + ).toBe(false); + + userEvent.click(screen.getByRole('tab', { name: /All charts/i })); + userEvent.dblClick(await screen.findByText('Table')); + expect(mockHistoryPush).toHaveBeenCalledWith( + '/explore/?viz_type=table&datasource=42__table', + ); +}); + +test('keeps the legacy no-options state when semantic layers are disabled', async () => { + fetchMock.clearHistory().removeRoutes(); + fetchMock.get(/\/api\/v1\/dataset\/\?q=.*/, { + body: { result: [], count: 0 }, + status: 200, + }); + + await renderComponent(); + userEvent.click(screen.getByRole('combobox', { name: 'Dataset' })); + + expect( + await screen.findByText('No data', { selector: '.ant-empty-description' }), + ).toBeInTheDocument(); + expect(screen.queryByText('Semantic View')).not.toBeInTheDocument(); +}); + +test('uses the exact dataset endpoint for URL preload with semantic layers enabled', async () => { + enableSemanticLayers(); + fetchMock.clearHistory().removeRoutes(); + fetchMock.get(/\/api\/v1\/dataset\/\?q=.*/, { + body: { result: legacyDatasetFixtures, count: 1 }, + status: 200, + }); + + const locationSpy = jest.spyOn(window, 'location', 'get').mockReturnValue({ + ...window.location, + search: '?dataset=shared_source', + } as Location); + + await renderComponent(); + + expect(await screen.findByText('shared_source')).toBeInTheDocument(); + // The preloaded selection is labelled the same way dropdown options are. + expect( + screen.getByText('Dataset', { selector: '.ant-tag' }), + ).toBeInTheDocument(); + expect( + fetchMock.callHistory.calls().some(call => { + const decodedUrl = decodeURIComponent(call.url); + return ( + decodedUrl.includes('/api/v1/dataset/') && + decodedUrl.includes('opr:eq') && + decodedUrl.includes('shared_source') + ); + }), + ).toBe(true); + expect( + fetchMock.callHistory + .calls() + .some(call => call.url.includes('/api/v1/datasource/')), + ).toBe(false); + + locationSpy.mockRestore(); +}); diff --git a/superset-frontend/src/pages/ChartCreation/index.tsx b/superset-frontend/src/pages/ChartCreation/index.tsx index a4151f6517ee..9564e07ca410 100644 --- a/superset-frontend/src/pages/ChartCreation/index.tsx +++ b/superset-frontend/src/pages/ChartCreation/index.tsx @@ -17,9 +17,8 @@ * under the License. */ import { ReactNode, useState, useEffect, useCallback, useMemo } from 'react'; -import rison from 'rison'; import { t } from '@apache-superset/core/translation'; -import { isDefined, JsonResponse, SupersetClient } from '@superset-ui/core'; +import { FeatureFlag, isDefined, isFeatureEnabled } from '@superset-ui/core'; import { styled, useTheme } from '@apache-superset/core/theme'; import { getUrlParam } from 'src/utils/urlUtils'; import { FilterPlugins, URL_PARAMS } from 'src/constants'; @@ -29,6 +28,7 @@ import { Button, Loading, Steps, + Tag, } from '@superset-ui/core/components'; import { propertyComparator } from '@superset-ui/core/components/Select/utils'; import withToasts from 'src/components/MessageToasts/withToasts'; @@ -43,6 +43,7 @@ import { Dataset, DatasetSelectLabel, } from 'src/features/datasets/DatasetSelectLabel'; +import { fetchDatasourceList } from 'src/features/datasets/fetchDatasourceList'; import { Icons } from '@superset-ui/core/components/Icons'; import { datasetLabel, @@ -54,6 +55,19 @@ export interface ChartCreationProps { addSuccessToast: (arg: string) => void; } +type DatasourceOption = { + id: number; + label: ReactNode; + value: string; + table_name: string; +}; + +/** + * AsyncSelect reports the chosen option as a LabeledValue, so only the + * fields it carries can be relied on after selection. + */ +type SelectedDatasource = Pick; + const ESTIMATED_NAV_HEIGHT = 56; const ELEMENTS_EXCEPT_VIZ_GALLERY = ESTIMATED_NAV_HEIGHT + 250; @@ -167,12 +181,52 @@ const StyledStepDescription = styled.div` `} `; +// The first column shrinks to zero so the name ellipsizes and the tag stays +// visible. AsyncSelect renders option labels inside a nowrap container, which +// is what makes that ellipsis take effect. +const StyledDatasourceOption = styled.div` + ${({ theme }) => ` + display: grid; + grid-template-columns: minmax(0, 1fr) auto; + gap: ${theme.sizeXS}px; + align-items: center; + width: 100%; + `} +`; + +/** + * Builds a picker option whose value carries the Explore datasource identity + * (`__table` or `__semantic_view`). Datasets and semantic views are + * numbered independently, so the type suffix is what keeps them apart. + */ +const createDatasourceOption = ( + item: Dataset, + withTypeTag: boolean, +): DatasourceOption => { + const isSemanticView = item.kind === 'semantic_view'; + const datasourceLabel = DatasetSelectLabel(item); + return { + id: item.id, + value: `${item.id}__${isSemanticView ? 'semantic_view' : 'table'}`, + table_name: item.table_name, + label: withTypeTag ? ( + + {datasourceLabel} + {isSemanticView ? t('Semantic View') : t('Dataset')} + + ) : ( + datasourceLabel + ), + }; +}; + export const ChartCreation = ({ user, addSuccessToast, }: ChartCreationProps) => { const theme = useTheme(); const history = useHistory(); + const semanticLayersEnabled = isFeatureEnabled(FeatureFlag.SemanticLayers); const canCreateDataset = useMemo( () => findPermission('can_write', 'Dataset', user.roles), @@ -185,8 +239,8 @@ export const ChartCreation = ({ ); const [datasource, setDatasource] = useState< - { label: string | ReactNode; value: string } | undefined - >(undefined); + SelectedDatasource | undefined + >(); const [vizType, setVizType] = useState(null); const [loading, setLoading] = useState(hasDatasetParam); @@ -203,10 +257,10 @@ export const ChartCreation = ({ history.push(exploreUrl()); }, [history, exploreUrl]); + // AsyncSelect's onChange is typed for every select value shape; narrow it + // to the labelled option this picker produces. const changeDatasource = useCallback( - (newDatasource: { label: string | ReactNode; value: string }) => { - setDatasource(newDatasource); - }, + (selected: SelectedDatasource) => setDatasource(selected), [], ); @@ -225,54 +279,34 @@ export const ChartCreation = ({ } }, [isBtnDisabled, gotoSlice]); + // Type tags only make sense once the list can mix datasets and semantic + // views, so they follow the feature flag rather than the endpoint used. + const toDatasourceOption = useCallback( + (item: Dataset) => createDatasourceOption(item, semanticLayersEnabled), + [semanticLayersEnabled], + ); + const loadDatasources = useCallback( - (search: string, page: number, pageSize: number, exactMatch = false) => { - const query = rison.encode({ - columns: [ - 'id', - 'table_name', - 'datasource_type', - 'database.database_name', - 'schema', - ], - filters: [ - { col: 'table_name', opr: exactMatch ? 'eq' : 'ct', value: search }, - ], - page, - page_size: pageSize, - order_column: 'table_name', - order_direction: 'asc', - }); - return SupersetClient.get({ - endpoint: `/api/v1/dataset/?q=${query}`, - }).then((response: JsonResponse) => { - const list: { - id: number; - label: string | ReactNode; - value: string; - table_name: string; - }[] = response.json.result.map((item: Dataset) => ({ - id: item.id, - value: `${item.id}__${item.datasource_type}`, - label: DatasetSelectLabel(item), - table_name: item.table_name, - })); - return { - data: list, - totalCount: response.json.count, - }; - }); - }, - [], + (search: string, page: number, pageSize: number) => + fetchDatasourceList(search, page, pageSize).then(({ result, count }) => ({ + data: result.map(toDatasourceOption), + totalCount: count, + })), + [toDatasourceOption], ); useEffect(() => { const params = new URLSearchParams(window.location.search).get('dataset'); if (params) { - loadDatasources(params, 0, 1, true) - .then(r => { - const newDatasource = r.data[0]; - setDatasource(newDatasource); + // The URL names a dataset that was just saved, so resolve it against + // datasets only: a semantic view with the same name must not win. + fetchDatasourceList(params, 0, 1, { + exactMatch: true, + datasetsOnly: true, + }) + .then(({ result }) => { + const [dataset] = result; + setDatasource(dataset && toDatasourceOption(dataset)); setLoading(false); }) .catch(() => { @@ -280,7 +314,7 @@ export const ChartCreation = ({ }); addSuccessToast(t('The dataset has been saved')); } - }, [loadDatasources, addSuccessToast]); + }, [toDatasourceOption, addSuccessToast]); const isButtonDisabled = isBtnDisabled(); const VIEW_INSTRUCTIONS_TEXT = t('view instructions'); diff --git a/superset-websocket/package-lock.json b/superset-websocket/package-lock.json index 761b9c8bd005..66ed0fbbff36 100644 --- a/superset-websocket/package-lock.json +++ b/superset-websocket/package-lock.json @@ -25,7 +25,7 @@ "@types/node": "^26.2.0", "@types/ws": "^8.18.1", "@typescript-eslint/eslint-plugin": "^8.67.0", - "@typescript-eslint/parser": "^8.67.0", + "@typescript-eslint/parser": "^8.68.0", "eslint": "^10.9.1", "eslint-config-prettier": "^10.1.8", "globals": "^17.11.0", @@ -1034,16 +1034,16 @@ } }, "node_modules/@typescript-eslint/parser": { - "version": "8.67.0", - "resolved": "https://registry.npmjs.org/@typescript-eslint/parser/-/parser-8.67.0.tgz", - "integrity": "sha512-fUBfTuuEulWqX6V8+O3PtScV01tzYYRUDTAirHFKoRAt7nOzoGiPt0M/bB47wWNy0coOOcgEwAMUtBpykMxl6w==", + "version": "8.68.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/parser/-/parser-8.68.0.tgz", + "integrity": "sha512-fHq2VC1kpyYfvEcbiMjOpySY4WS7voEp89yAThrHRX5sm9j2lzYppCb2umFMEed4fWcyeLjHxrz0mpjNBaBxMQ==", "dev": true, "license": "MIT", "dependencies": { - "@typescript-eslint/scope-manager": "8.67.0", - "@typescript-eslint/types": "8.67.0", - "@typescript-eslint/typescript-estree": "8.67.0", - "@typescript-eslint/visitor-keys": "8.67.0", + "@typescript-eslint/scope-manager": "8.68.0", + "@typescript-eslint/types": "8.68.0", + "@typescript-eslint/typescript-estree": "8.68.0", + "@typescript-eslint/visitor-keys": "8.68.0", "debug": "^4.4.3" }, "engines": { @@ -1058,6 +1058,136 @@ "typescript": ">=4.8.4 <6.1.0" } }, + "node_modules/@typescript-eslint/parser/node_modules/@typescript-eslint/project-service": { + "version": "8.68.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/project-service/-/project-service-8.68.0.tgz", + "integrity": "sha512-5GQtWZCXFcFYux955pvoS02WLc49pXNlvIxocKjS0clvwo3in1RdlzVKyiqQH9vE5AKWFLTaUgeQkOrTS+0Qxw==", + "dev": true, + "license": "MIT", + "dependencies": { + "@typescript-eslint/tsconfig-utils": "^8.68.0", + "@typescript-eslint/types": "^8.68.0", + "debug": "^4.4.3" + }, + "engines": { + "node": "^18.18.0 || ^20.9.0 || >=21.1.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/typescript-eslint" + }, + "peerDependencies": { + "typescript": ">=4.8.4 <6.1.0" + } + }, + "node_modules/@typescript-eslint/parser/node_modules/@typescript-eslint/scope-manager": { + "version": "8.68.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/scope-manager/-/scope-manager-8.68.0.tgz", + "integrity": "sha512-T5eXpcaJNg8bhjHJ8Rjp68Vq/QBteYtTKY8TZqVNPaUbuz0f6jI9t6aDkylwvalpAB9XTTFeFOjrjXAZ3YvmVA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@typescript-eslint/types": "8.68.0", + "@typescript-eslint/visitor-keys": "8.68.0" + }, + "engines": { + "node": "^18.18.0 || ^20.9.0 || >=21.1.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/typescript-eslint" + } + }, + "node_modules/@typescript-eslint/parser/node_modules/@typescript-eslint/tsconfig-utils": { + "version": "8.68.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/tsconfig-utils/-/tsconfig-utils-8.68.0.tgz", + "integrity": "sha512-F7zrGQfiJHojPwi8vhxZQC1tWtJzvL74cK/nqri2lk8YUXvYaYwl263xOJ69jDWPUk1hmcdoayFwk9lX09npVw==", + "dev": true, + "license": "MIT", + "engines": { + "node": "^18.18.0 || ^20.9.0 || >=21.1.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/typescript-eslint" + }, + "peerDependencies": { + "typescript": ">=4.8.4 <6.1.0" + } + }, + "node_modules/@typescript-eslint/parser/node_modules/@typescript-eslint/types": { + "version": "8.68.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/types/-/types-8.68.0.tgz", + "integrity": "sha512-9RnpsGJjrAllCMefGVVsImJM24YurhC0Q1h4UbvivtvOqXmR/vEJge2OoE++z9m6hyg8T1Q8t5SNT6tHSbrxcg==", + "dev": true, + "license": "MIT", + "engines": { + "node": "^18.18.0 || ^20.9.0 || >=21.1.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/typescript-eslint" + } + }, + "node_modules/@typescript-eslint/parser/node_modules/@typescript-eslint/typescript-estree": { + "version": "8.68.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/typescript-estree/-/typescript-estree-8.68.0.tgz", + "integrity": "sha512-OKKsD0tYmoNiU5PW2zehO1yO56jYOm1ShYlxon/Z0SJNidAkdVg86eg9ruRuoXf8xfnuWZGbwDsStkoXbZtIIA==", + "dev": true, + "license": "MIT", + "dependencies": { + "@typescript-eslint/project-service": "8.68.0", + "@typescript-eslint/tsconfig-utils": "8.68.0", + "@typescript-eslint/types": "8.68.0", + "@typescript-eslint/visitor-keys": "8.68.0", + "debug": "^4.4.3", + "minimatch": "^10.2.2", + "semver": "^7.7.3", + "tinyglobby": "^0.2.15", + "ts-api-utils": "^2.5.0" + }, + "engines": { + "node": "^18.18.0 || ^20.9.0 || >=21.1.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/typescript-eslint" + }, + "peerDependencies": { + "typescript": ">=4.8.4 <6.1.0" + } + }, + "node_modules/@typescript-eslint/parser/node_modules/@typescript-eslint/visitor-keys": { + "version": "8.68.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/visitor-keys/-/visitor-keys-8.68.0.tgz", + "integrity": "sha512-YR65gGdGvTUAWLldC3xLOvOzamdGzB4A5/N8rehEaHs3Zvoe39BhgY+u0SPch1OvrVTfLcc55wsSgK2NcnTS/A==", + "dev": true, + "license": "MIT", + "dependencies": { + "@typescript-eslint/types": "8.68.0", + "eslint-visitor-keys": "^5.0.0" + }, + "engines": { + "node": "^18.18.0 || ^20.9.0 || >=21.1.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/typescript-eslint" + } + }, + "node_modules/@typescript-eslint/parser/node_modules/eslint-visitor-keys": { + "version": "5.0.1", + "resolved": "https://registry.npmjs.org/eslint-visitor-keys/-/eslint-visitor-keys-5.0.1.tgz", + "integrity": "sha512-tD40eHxA35h0PEIZNeIjkHoDR4YjjJp34biM0mDvplBe//mB+IHCqHDGV7pxF+7MklTvighcCPPZC7ynWyjdTA==", + "dev": true, + "license": "Apache-2.0", + "engines": { + "node": "^20.19.0 || ^22.13.0 || >=24" + }, + "funding": { + "url": "https://opencollective.com/eslint" + } + }, "node_modules/@typescript-eslint/project-service": { "version": "8.67.0", "resolved": "https://registry.npmjs.org/@typescript-eslint/project-service/-/project-service-8.67.0.tgz", @@ -3172,6 +3302,31 @@ "typescript": ">=4.8.4 <6.1.0" } }, + "node_modules/typescript-eslint/node_modules/@typescript-eslint/parser": { + "version": "8.67.0", + "resolved": "https://registry.npmjs.org/@typescript-eslint/parser/-/parser-8.67.0.tgz", + "integrity": "sha512-fUBfTuuEulWqX6V8+O3PtScV01tzYYRUDTAirHFKoRAt7nOzoGiPt0M/bB47wWNy0coOOcgEwAMUtBpykMxl6w==", + "dev": true, + "license": "MIT", + "dependencies": { + "@typescript-eslint/scope-manager": "8.67.0", + "@typescript-eslint/types": "8.67.0", + "@typescript-eslint/typescript-estree": "8.67.0", + "@typescript-eslint/visitor-keys": "8.67.0", + "debug": "^4.4.3" + }, + "engines": { + "node": "^18.18.0 || ^20.9.0 || >=21.1.0" + }, + "funding": { + "type": "opencollective", + "url": "https://opencollective.com/typescript-eslint" + }, + "peerDependencies": { + "eslint": "^8.57.0 || ^9.0.0 || ^10.0.0", + "typescript": ">=4.8.4 <6.1.0" + } + }, "node_modules/undici-types": { "version": "8.3.0", "resolved": "https://registry.npmjs.org/undici-types/-/undici-types-8.3.0.tgz", diff --git a/superset-websocket/package.json b/superset-websocket/package.json index 9b760672731a..0d1684917829 100644 --- a/superset-websocket/package.json +++ b/superset-websocket/package.json @@ -33,7 +33,7 @@ "@types/node": "^26.2.0", "@types/ws": "^8.18.1", "@typescript-eslint/eslint-plugin": "^8.67.0", - "@typescript-eslint/parser": "^8.67.0", + "@typescript-eslint/parser": "^8.68.0", "eslint": "^10.9.1", "eslint-config-prettier": "^10.1.8", "globals": "^17.11.0", diff --git a/superset/commands/deletion_retention/force_purge.py b/superset/commands/deletion_retention/force_purge.py index 872a3e8763e0..cd07a5e58349 100644 --- a/superset/commands/deletion_retention/force_purge.py +++ b/superset/commands/deletion_retention/force_purge.py @@ -74,12 +74,14 @@ def __init__( model_cls: type[SoftDeleteMixin] | None = None, require_archived: bool = False, require_audit: bool = False, + confirmed_impact_token: str | None = None, ) -> None: self._uuid: str = uuid self._actor: str = actor self._model_cls = model_cls self._require_archived = require_archived self._require_audit = require_audit + self._confirmed_impact_token: str | None = confirmed_impact_token def _resolve(self) -> SoftDeleteMixin | None: """Find the entity by UUID, visibility-filter bypassed. @@ -164,6 +166,7 @@ def run(self) -> dict[str, Any]: entity, enforce_window=False, require_archived=self._require_archived, + confirmed_impact_token=self._confirmed_impact_token, ) # Commit AFTER the suppression block: Continuum executes its # pending association statements during flush/commit, so the diff --git a/superset/commands/deletion_retention/purge_cascade.py b/superset/commands/deletion_retention/purge_cascade.py index 4b370e010722..cb8b4694b040 100644 --- a/superset/commands/deletion_retention/purge_cascade.py +++ b/superset/commands/deletion_retention/purge_cascade.py @@ -52,6 +52,11 @@ from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session +from superset.commands.deletion_retention.purge_impact import ( + collect_dataset_purge_impact, + DatasetPurgeImpact, + PurgeImpactChangedError, +) from superset.commands.deletion_retention.purge_policy import ( BlockerReason, get_purge_policy, @@ -192,6 +197,7 @@ def cascade_hard_delete( enforce_window: bool, cutoff: datetime | None = None, require_archived: bool = False, + confirmed_impact_token: str | None = None, ) -> CascadeResult: """Remove *entity* and everything that depends on it in one transaction. @@ -224,6 +230,7 @@ def cascade_hard_delete( removed_dashboard_slices = 0 version_rows = 0 permission_name: str | None = None + confirmed_impact: DatasetPurgeImpact | None = None try: with session.begin_nested(): @@ -241,6 +248,11 @@ def cascade_hard_delete( if session.execute(claim.with_for_update()).scalar_one_or_none() is None: raise PurgeRaceLostError + if entity_type == "dataset" and confirmed_impact_token is not None: + confirmed_impact = collect_dataset_purge_impact(session, entity_id) + if confirmed_impact.impact_token != confirmed_impact_token: + raise PurgeImpactChangedError(confirmed_impact) + policy.validate(session, policy, entity_id) # Captured under the lock: the row is claimed, so the identity # the permission name is built from can no longer change. @@ -248,8 +260,10 @@ def cascade_hard_delete( removed_dashboard_slices = policy.count_dashboard_slices( session, policy, entity_id ) - dangling_chart_uuids = policy.collect_dangling_chart_uuids( - session, policy, entity_id + dangling_chart_uuids = ( + [chart.uuid for chart in confirmed_impact.charts] + if confirmed_impact is not None + else policy.collect_dangling_chart_uuids(session, policy, entity_id) ) policy.delete_associations(session, policy, entity_id) diff --git a/superset/commands/deletion_retention/purge_impact.py b/superset/commands/deletion_retention/purge_impact.py new file mode 100644 index 000000000000..6dc22197e8cf --- /dev/null +++ b/superset/commands/deletion_retention/purge_impact.py @@ -0,0 +1,128 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Authoritative dependency snapshots for archived dataset purges.""" + +from __future__ import annotations + +import hashlib +from dataclasses import dataclass + +import sqlalchemy as sa +from sqlalchemy.orm import Session + +from superset.models.dashboard import Dashboard, dashboard_slices +from superset.models.helpers import skip_visibility_filter +from superset.models.slice import Slice + + +@dataclass(frozen=True) +class DatasetImpactObject: + """Scalar identity and display data for one dependent object.""" + + id: int + uuid: str + name: str | None + archived: bool + + +@dataclass(frozen=True) +class DatasetPurgeImpact: + """Complete dependent identities at one point in time.""" + + impact_token: str + charts: tuple[DatasetImpactObject, ...] + dashboards: tuple[DatasetImpactObject, ...] + + +class PurgeImpactChangedError(Exception): + """Raised when a dataset's dependencies differ from those confirmed.""" + + def __init__(self, impact: DatasetPurgeImpact) -> None: + super().__init__("Dataset dependencies changed; review them before deleting") + self.impact: DatasetPurgeImpact = impact + + +def _impact_token( + charts: tuple[DatasetImpactObject, ...], + dashboards: tuple[DatasetImpactObject, ...], +) -> str: + """Return a deterministic, versioned fingerprint of dependent identities.""" + canonical: str = "\n".join( + sorted(f"chart:{chart.uuid}" for chart in charts) + + sorted(f"dashboard:{dashboard.uuid}" for dashboard in dashboards) + ) + digest: str = hashlib.sha256(canonical.encode()).hexdigest() + return f"v1:{digest}" + + +def collect_dataset_purge_impact( + session: Session, dataset_id: int +) -> DatasetPurgeImpact: + """Collect live and archived charts and distinct dashboards for a dataset.""" + with skip_visibility_filter(session, Slice, Dashboard): + chart_rows: list[tuple[int, object, str | None, object | None]] = list( + session.execute( + sa.select(Slice.id, Slice.uuid, Slice.slice_name, Slice.deleted_at) + .where(Slice.datasource_type == "table") + .where(Slice.datasource_id == dataset_id) + .order_by(Slice.uuid) + ).tuples() + ) + dashboard_rows: list[tuple[int, object, str | None, object | None]] = list( + session.execute( + sa.select( + Dashboard.id, + Dashboard.uuid, + Dashboard.dashboard_title, + Dashboard.deleted_at, + ) + .join( + dashboard_slices, + dashboard_slices.c.dashboard_id == Dashboard.id, + ) + .join(Slice, Slice.id == dashboard_slices.c.slice_id) + .where(Slice.datasource_type == "table") + .where(Slice.datasource_id == dataset_id) + .distinct() + .order_by(Dashboard.uuid) + ).tuples() + ) + + charts: tuple[DatasetImpactObject, ...] = tuple( + DatasetImpactObject( + id=chart_id, + uuid=str(chart_uuid), + name=chart_name, + archived=deleted_at is not None, + ) + for chart_id, chart_uuid, chart_name, deleted_at in chart_rows + ) + dashboard_by_uuid: dict[str, DatasetImpactObject] = { + str(dashboard_uuid): DatasetImpactObject( + id=dashboard_id, + uuid=str(dashboard_uuid), + name=dashboard_name, + archived=deleted_at is not None, + ) + for dashboard_id, dashboard_uuid, dashboard_name, deleted_at in dashboard_rows + } + dashboards: tuple[DatasetImpactObject, ...] = tuple(dashboard_by_uuid.values()) + return DatasetPurgeImpact( + impact_token=_impact_token(charts, dashboards), + charts=charts, + dashboards=dashboards, + ) diff --git a/superset/commands/deletion_retention/purge_policy.py b/superset/commands/deletion_retention/purge_policy.py index 71cb896159ad..67b266462577 100644 --- a/superset/commands/deletion_retention/purge_policy.py +++ b/superset/commands/deletion_retention/purge_policy.py @@ -553,6 +553,9 @@ def declare( fk("slices", "report_schedule", "id", "chart_id", "inbound"), version("slices", "slices_version"), relationship("slices", "tables", "manytoone", "table"), + relationship( + "slices", "semantic_views", "manytoone", "semantic_view" + ), fk("chart_editors", "slices", "chart_id", "id", "outbound"), fk("chart_editors", "subjects", "subject_id", "id", "outbound"), fk("chart_viewers", "slices", "chart_id", "id", "outbound"), @@ -588,6 +591,7 @@ def declare( DependencyClassification.PRESERVE, DependencyClassification.PRESERVE, DependencyClassification.PRESERVE, + DependencyClassification.PRESERVE, ), (tag_cleanup, chart_membership_versions), # Keyed by related table; the audit code is declared, not derived. @@ -988,17 +992,13 @@ def dangling_chart_uuids( """Return chart UUIDs left by the dataset preservation policy.""" if policy.entity_type != "dataset": return [] - # avoid circular import: the chart model participates in registry assembly - from superset.models.slice import Slice + from superset.commands.deletion_retention.purge_impact import ( + collect_dataset_purge_impact, + DatasetPurgeImpact, + ) - return [ - str(chart_uuid) - for (chart_uuid,) in session.execute( - sa.select(Slice.uuid) - .where(Slice.datasource_id == entity_id) - .where(Slice.datasource_type == "table") - ) - ] + impact: DatasetPurgeImpact = collect_dataset_purge_impact(session, entity_id) + return [chart.uuid for chart in impact.charts] def delete_associations( diff --git a/superset/commands/purge.py b/superset/commands/purge.py index b6554bc7f2ed..43ad6a8d1295 100644 --- a/superset/commands/purge.py +++ b/superset/commands/purge.py @@ -25,22 +25,35 @@ """ import logging +from collections.abc import Callable from dataclasses import dataclass -from typing import Any +from typing import Any, TypeAlias from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.orm import Session -from superset import is_feature_enabled, security_manager +from superset import db, is_feature_enabled, security_manager from superset.commands.base import BaseCommand from superset.commands.deletion_retention.force_purge import ForcePurgeCommand +from superset.commands.deletion_retention.purge_impact import ( + collect_dataset_purge_impact, + DatasetImpactObject, + DatasetPurgeImpact, + PurgeImpactChangedError, +) +from superset.connectors.sqla.models import SqlaTable from superset.daos.base import BaseDAO from superset.daos.exceptions import DAODeleteFailedError from superset.exceptions import SupersetSecurityException -from superset.models.helpers import SoftDeleteMixin +from superset.models.dashboard import Dashboard +from superset.models.helpers import skip_visibility_filter, SoftDeleteMixin +from superset.models.slice import Slice from superset.tasks.utils import get_current_user logger = logging.getLogger(__name__) +ImpactCollector: TypeAlias = Callable[[Session, SoftDeleteMixin], DatasetPurgeImpact] + #: Recorded when the audit trail cannot name the acting user. The purge routes #: are ``@protect()``-ed, so this should be unreachable; it exists so an #: anomaly is visible as one rather than disguised as a plausible username. @@ -60,14 +73,21 @@ class SoftDeleteBinding: not_found: type[Exception] forbidden: type[Exception] delete_failed: type[Exception] + impact_collector: ImpactCollector | None = None class PurgeArchivedCommand(BaseCommand): """Permanently delete a single soft-deleted entity, by UUID.""" - def __init__(self, model_uuid: str, binding: SoftDeleteBinding) -> None: + def __init__( + self, + model_uuid: str, + binding: SoftDeleteBinding, + confirmed_impact_token: str | None = None, + ) -> None: self._model_uuid = model_uuid self._binding = binding + self._confirmed_impact_token: str | None = confirmed_impact_token #: The authorized entity, resolved by ``validate()``. ``BaseCommand`` #: fixes ``validate()``'s return type as ``None``, so the model is #: handed to ``run()`` here rather than returned. @@ -78,6 +98,13 @@ def run(self) -> None: model = self._model if model is None: # pragma: no cover — validate() raises or sets it raise self._binding.not_found(f"No row with uuid={self._model_uuid!r}") + if self._binding.impact_collector is not None: + impact: DatasetPurgeImpact = self._binding.impact_collector( + db.session, model + ) + if impact.impact_token != self._confirmed_impact_token: + raise PurgeImpactChangedError(impact) + try: # ForcePurgeCommand owns the cascade + commit + audit. # @@ -98,6 +125,7 @@ def run(self) -> None: # the CLI's fail-open default is an operator-trust decision # that does not extend to REST principals. require_audit=True, + confirmed_impact_token=self._confirmed_impact_token, ).run() except (SQLAlchemyError, DAODeleteFailedError) as ex: # Deliberately narrow: a database or DAO failure is a real @@ -175,3 +203,82 @@ def validate(self) -> None: except SupersetSecurityException as ex: raise self._binding.forbidden() from ex self._model = model + + def preview_dataset_impact(self) -> DatasetPurgeImpact: + """Return the authorized impact snapshot for an archived dataset.""" + self.validate() + model: SoftDeleteMixin | None = self._model + collector: ImpactCollector | None = self._binding.impact_collector + if model is None or collector is None: + raise self._binding.not_found("The purge target is not a dataset") + return collector(db.session, model) + + +def collect_dataset_impact( + session: Session, model: SoftDeleteMixin +) -> DatasetPurgeImpact: + """Collect purge impact for a dataset binding.""" + if not isinstance(model, SqlaTable): + raise TypeError("Dataset purge binding resolved a non-dataset model") + return collect_dataset_purge_impact(session, model.id) + + +def serialize_dataset_purge_impact(impact: DatasetPurgeImpact) -> dict[str, Any]: + """Apply object access control and serialize a complete impact snapshot.""" + chart_ids: list[int] = [item.id for item in impact.charts] + dashboard_ids: list[int] = [item.id for item in impact.dashboards] + with skip_visibility_filter(db.session, Slice, Dashboard): + charts: dict[int, Slice] = { + chart.id: chart + for chart in db.session.query(Slice).filter(Slice.id.in_(chart_ids)).all() + } + dashboards: dict[int, Dashboard] = { + dashboard.id: dashboard + for dashboard in db.session.query(Dashboard) + .filter(Dashboard.id.in_(dashboard_ids)) + .all() + } + + chart_result: list[dict[str, Any]] = [] + for item in impact.charts: + chart: Slice | None = charts.get(item.id) + if chart is not None and security_manager.can_access_chart(chart): + chart_result.append( + _serialize_impact_object(item, chart.url, fallback="Untitled chart") + ) + + dashboard_result: list[dict[str, Any]] = [] + for item in impact.dashboards: + dashboard: Dashboard | None = dashboards.get(item.id) + if dashboard is not None and security_manager.can_access_dashboard(dashboard): + dashboard_result.append( + _serialize_impact_object( + item, dashboard.url, fallback="Untitled dashboard" + ) + ) + + return { + "impact_token": impact.impact_token, + "charts": { + "count": len(impact.charts), + "restricted_count": len(impact.charts) - len(chart_result), + "result": chart_result, + }, + "dashboards": { + "count": len(impact.dashboards), + "restricted_count": len(impact.dashboards) - len(dashboard_result), + "result": dashboard_result, + }, + } + + +def _serialize_impact_object( + item: DatasetImpactObject, live_url: str, *, fallback: str +) -> dict[str, Any]: + """Serialize one authorized object without linking an archived target.""" + return { + "uuid": item.uuid, + "name": item.name or fallback, + "archived": item.archived, + "url": None if item.archived else live_url, + } diff --git a/superset/connectors/sqla/models.py b/superset/connectors/sqla/models.py index 2409c2ffae47..4b11b48a84ff 100644 --- a/superset/connectors/sqla/models.py +++ b/superset/connectors/sqla/models.py @@ -1236,7 +1236,11 @@ def get_sqla_col( expression = self._validate_stored_expression(expression) col = literal_column(expression, type_=type_) else: - col = column(self.column_name, type_=type_) + identifier = db_engine_spec.prepare_identifier( + cast(str, self.column_name), + normalize_columns=bool(getattr(self.table, "normalize_columns", False)), + ) + col = column(identifier, type_=type_) col = self.database.make_sqla_column_compatible(col, label) return col @@ -1266,12 +1270,15 @@ def get_timestamp_expression( pdf = self.python_date_format is_epoch = pdf in ("epoch_s", "epoch_ms") - column_spec = self.db_engine_spec.get_column_spec( - self.type, db_extra=self.db_extra - ) + db_engine_spec = self.db_engine_spec + column_spec = db_engine_spec.get_column_spec(self.type, db_extra=self.db_extra) type_ = column_spec.sqla_type if column_spec else DateTime if not self.expression and not time_grain and not is_epoch: - sqla_col = column(self.column_name, type_=type_) + identifier = db_engine_spec.prepare_identifier( + cast(str, self.column_name), + normalize_columns=bool(getattr(self.table, "normalize_columns", False)), + ) + sqla_col = column(identifier, type_=type_) return self.database.make_sqla_column_compatible(sqla_col, label) if expression := self.expression: if template_processor: @@ -1296,7 +1303,11 @@ def get_timestamp_expression( expression = self._validate_stored_expression(expression) col = literal_column(expression, type_=type_) else: - col = column(self.column_name, type_=type_) + identifier = db_engine_spec.prepare_identifier( + cast(str, self.column_name), + normalize_columns=bool(getattr(self.table, "normalize_columns", False)), + ) + col = column(identifier, type_=type_) if ( apply_dataset_offset and time_grain @@ -1933,7 +1944,12 @@ def adhoc_metric_to_sqla( template_processor=template_processor ) else: - sqla_column = column(column_name) + sqla_column = column( + self.db_engine_spec.prepare_identifier( + column_name, + normalize_columns=bool(self.normalize_columns), + ) + ) if isinstance(aggregate, str) and aggregate in self.sqla_aggregations: sqla_metric = self.sqla_aggregations[aggregate](sqla_column) diff --git a/superset/daos/datasource.py b/superset/daos/datasource.py index 726e91cf42fc..f96aa83c7ec5 100644 --- a/superset/daos/datasource.py +++ b/superset/daos/datasource.py @@ -205,9 +205,13 @@ def paginate_combined_query( sort_col = combined.c[sort_col_name] ordered_col = sort_col.desc() if order_direction == "desc" else sort_col.asc() + # None of the sortable columns is unique across the union (a dataset + # and a semantic view may share a name, two datasets may share a + # changed_on), so offset pagination needs a total order or rows can + # repeat or vanish at page boundaries. rows = db.session.execute( select(combined.c.item_id, combined.c.source_type) - .order_by(ordered_col) + .order_by(ordered_col, combined.c.source_type, combined.c.item_id) .offset(page * page_size) .limit(page_size) ).fetchall() diff --git a/superset/datasets/api.py b/superset/datasets/api.py index 21607bf3e39a..00eb4b110b15 100644 --- a/superset/datasets/api.py +++ b/superset/datasets/api.py @@ -56,10 +56,19 @@ from superset.commands.dataset.restore import RestoreDatasetCommand from superset.commands.dataset.update import UpdateDatasetCommand from superset.commands.dataset.warm_up_cache import DatasetWarmUpCacheCommand +from superset.commands.deletion_retention.purge_impact import ( + DatasetPurgeImpact, + PurgeImpactChangedError, +) from superset.commands.exceptions import CommandException from superset.commands.importers.exceptions import NoValidFilesFoundError from superset.commands.importers.v1.utils import get_contents_from_bundle -from superset.commands.purge import PurgeArchivedCommand, SoftDeleteBinding +from superset.commands.purge import ( + collect_dataset_impact, + PurgeArchivedCommand, + serialize_dataset_purge_impact, + SoftDeleteBinding, +) from superset.connectors.sqla.models import SqlaTable from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod from superset.daos.dashboard import DashboardDAO @@ -78,6 +87,8 @@ DatasetDrillInfoSchema, DatasetDuplicateSchema, DatasetPostSchema, + DatasetPurgeImpactSchema, + DatasetPurgeRequestSchema, DatasetPutSchema, DatasetRelatedObjectsResponse, get_delete_ids_schema, @@ -134,6 +145,7 @@ not_found=DatasetNotFoundError, forbidden=DatasetForbiddenError, delete_failed=DatasetDeleteFailedError, + impact_collector=collect_dataset_impact, ) @@ -156,6 +168,7 @@ class DatasetRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): "restore": "write", "restore_version": "write", "purge": "write", + "purge_impact": "write", } include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | { RouteMethod.EXPORT, @@ -165,6 +178,7 @@ class DatasetRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): "bulk_delete", "restore", "purge", + "purge_impact", "refresh", "related_objects", "duplicate", @@ -402,6 +416,8 @@ class DatasetRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi): DatasetCacheWarmUpRequestSchema, DatasetCacheWarmUpResponseSchema, DatasetRelatedObjectsResponse, + DatasetPurgeImpactSchema, + DatasetPurgeRequestSchema, DatasetDuplicateSchema, GetOrCreateDatasetSchema, VersionListItemSchema, @@ -1300,6 +1316,63 @@ def restore(self, uuid: str) -> Response: ) return self.response_422(message=str(ex)) + @expose("//purge-impact", methods=("GET",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: ( + f"{self.__class__.__name__}.purge_impact" + ), + log_to_statsd=False, + ) + def purge_impact(self, uuid: str) -> Response: + """Preview the dependency impact of purging an archived dataset. + --- + get: + summary: Preview the dependency impact of purging an archived dataset + description: >- + Report the charts and dashboards that depend on an archived + dataset, with an impact token that must be echoed back on the + purge request. Limited to owners and admins (same audience as + restore). + parameters: + - in: path + schema: + type: string + format: uuid + name: uuid + responses: + 200: + description: Dependency impact of purging the dataset + content: + application/json: + schema: + $ref: '#/components/schemas/DatasetPurgeImpactSchema' + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 500: + $ref: '#/components/responses/500' + """ + try: + impact: DatasetPurgeImpact = PurgeArchivedCommand( + uuid, _DATASET_PURGE_BINDING + ).preview_dataset_impact() + return self.response(200, **serialize_dataset_purge_impact(impact)) + except DatasetNotFoundError: + return self.response_404() + except DatasetForbiddenError: + return self.response_403() + except Exception: # noqa: BLE001 + logger.exception("Unable to collect dataset purge impact") + return self.response_500( + message="The dependency impact could not be determined" + ) + @expose("//purge", methods=("POST",)) @protect() @safe @@ -1308,6 +1381,7 @@ def restore(self, uuid: str) -> Response: action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.purge", log_to_statsd=False, ) + @requires_json def purge(self, uuid: str) -> Response: """Permanently delete a soft-deleted (archived) dataset. --- @@ -1322,6 +1396,13 @@ def purge(self, uuid: str) -> Response: type: string format: uuid name: uuid + requestBody: + description: Confirmed dependency impact + required: true + content: + application/json: + schema: + $ref: '#/components/schemas/DatasetPurgeRequestSchema' responses: 200: description: Dataset permanently deleted @@ -1332,20 +1413,52 @@ def purge(self, uuid: str) -> Response: properties: message: type: string + 400: + $ref: '#/components/responses/400' 401: $ref: '#/components/responses/401' 403: $ref: '#/components/responses/403' 404: $ref: '#/components/responses/404' + 409: + description: Dataset dependency impact changed + content: + application/json: + schema: + type: object + required: [message, reason, impact] + properties: + message: + type: string + reason: + type: string + enum: [purge_impact_changed] + impact: + $ref: '#/components/schemas/DatasetPurgeImpactSchema' 422: $ref: '#/components/responses/422' 500: $ref: '#/components/responses/500' """ try: - PurgeArchivedCommand(uuid, _DATASET_PURGE_BINDING).run() + body: dict[str, Any] = DatasetPurgeRequestSchema().load(request.json) + confirmed_impact_token: str = body["confirmed_impact_token"] + PurgeArchivedCommand( + uuid, + _DATASET_PURGE_BINDING, + confirmed_impact_token=confirmed_impact_token, + ).run() return self.response(200, message="OK") + except ValidationError as ex: + return self.response_400(message=ex.messages) + except PurgeImpactChangedError as ex: + return self.response( + 409, + message=str(ex), + reason="purge_impact_changed", + impact=serialize_dataset_purge_impact(ex.impact), + ) except DatasetNotFoundError: return self.response_404() except DatasetForbiddenError: diff --git a/superset/datasets/schemas.py b/superset/datasets/schemas.py index 17425bee014c..0cb9c730e5da 100644 --- a/superset/datasets/schemas.py +++ b/superset/datasets/schemas.py @@ -28,7 +28,7 @@ validates_schema, ValidationError, ) -from marshmallow.validate import Length, OneOf +from marshmallow.validate import Length, OneOf, Range from superset import security_manager from superset.connectors.sqla.models import SqlaTable @@ -259,6 +259,58 @@ class DatasetRelatedObjectsResponse(Schema): dashboards = fields.Nested(DatasetRelatedDashboards) +class DatasetPurgeRequestSchema(Schema): + """Validate a dataset purge confirmation payload.""" + + confirmed_impact_token: fields.String = fields.String( + required=True, + allow_none=False, + validate=Length(min=1), + ) + + +class DatasetPurgeImpactObjectSchema(Schema): + """Describe one dependent object visible to the caller.""" + + uuid: fields.UUID = fields.UUID(required=True) + name: fields.String = fields.String(required=True) + archived: fields.Boolean = fields.Boolean(required=True) + url: fields.String = fields.String(required=True, allow_none=True) + + +class DatasetPurgeImpactCollectionSchema(Schema): + """Validate totals and visible results for one dependent object type.""" + + count: fields.Integer = fields.Integer(required=True, validate=Range(min=0)) + restricted_count: fields.Integer = fields.Integer( + required=True, validate=Range(min=0) + ) + result: fields.List = fields.List( + fields.Nested(DatasetPurgeImpactObjectSchema), required=True + ) + + @validates_schema + def validate_totals(self, data: dict[str, Any], **kwargs: Any) -> None: + """Require visible and restricted records to equal the total.""" + count: int = data["count"] + restricted_count: int = data["restricted_count"] + result: list[dict[str, Any]] = data["result"] + if restricted_count > count or len(result) + restricted_count != count: + raise ValidationError("Impact totals do not match the result") + + +class DatasetPurgeImpactSchema(Schema): + """Describe the authoritative, access-filtered dataset purge impact.""" + + impact_token: fields.String = fields.String(required=True) + charts: fields.Nested = fields.Nested( + DatasetPurgeImpactCollectionSchema, required=True + ) + dashboards: fields.Nested = fields.Nested( + DatasetPurgeImpactCollectionSchema, required=True + ) + + class ImportV1ColumnSchema(Schema): # pylint: disable=unused-argument @pre_load diff --git a/superset/db_engine_specs/base.py b/superset/db_engine_specs/base.py index 14fcce0642ce..34b4e4168a67 100644 --- a/superset/db_engine_specs/base.py +++ b/superset/db_engine_specs/base.py @@ -101,6 +101,7 @@ generate_code_challenge, generate_code_verifier, get_oauth2_redirect_uri, + is_oauth2_retry_active, ) if TYPE_CHECKING: @@ -976,7 +977,18 @@ def get_oauth2_fresh_token( else requests.post(uri, json=req_body, timeout=timeout) ) if response.status_code in (400, 401, 403): - raise OAuth2TokenRefreshError() + try: + payload = response.json() + if not isinstance(payload, dict): + payload = json.loads(response.text) + error = payload.get("error") + except (ValueError, TypeError, AttributeError): + error = None + # RFC 6749 defines invalid_grant for an invalid, expired, or revoked + # refresh token. Other error responses can be transient or indicate a + # client configuration problem and must not invalidate stored tokens. + if error == "invalid_grant": + raise OAuth2TokenRefreshError() response.raise_for_status() return response.json() @@ -1811,7 +1823,7 @@ def execute_with_cursor( ) if cancel_query_id is not None: query.set_extra_json_key(QUERY_CANCEL_KEY, cancel_query_id) - db.session.commit() + db.session.commit() # pylint: disable=consider-using-transaction logger.debug("Query %d: Handling cursor", query.id) cls.handle_cursor(cursor, query) @@ -2387,7 +2399,11 @@ def execute( # pylint: disable=unused-argument try: cursor.execute(query) except Exception as ex: - if database.is_oauth2_enabled() and cls.needs_oauth2(ex): + if ( + not is_oauth2_retry_active() + and database.is_oauth2_enabled() + and cls.needs_oauth2(ex) + ): cls.start_oauth2_dance(database) raise cls.get_dbapi_mapped_exception(ex) from ex @@ -2959,6 +2975,19 @@ def denormalize_name(cls, dialect: Dialect, name: str) -> str: return name + @classmethod + def prepare_identifier( + cls, + name: str, + normalize_columns: bool = False, + ) -> str: + """ + Prepare a physical identifier for SQLAlchemy column construction. + + The default preserves SQLAlchemy's automatic identifier-quoting behavior. + """ + return name + @classmethod def quote_table(cls, table: Table, dialect: Dialect) -> str: """ diff --git a/superset/db_engine_specs/gsheets.py b/superset/db_engine_specs/gsheets.py index c932299f0901..7fd1ac1156b1 100644 --- a/superset/db_engine_specs/gsheets.py +++ b/superset/db_engine_specs/gsheets.py @@ -393,7 +393,22 @@ def validate_parameters( # On create the encrypted credentials are a string, # at all other times they are a dict if isinstance(encrypted_credentials, str): - encrypted_credentials = json.loads(encrypted_credentials) + try: + encrypted_credentials = json.loads(encrypted_credentials) + except json.JSONDecodeError: + errors.append( + SupersetError( + message=( + "The service account credentials are not valid JSON. " + "Please check that the field contains a valid service " + "account key." + ), + error_type=SupersetErrorType.INVALID_PAYLOAD_FORMAT_ERROR, + level=ErrorLevel.ERROR, + extra={"invalid": ["service_account_info"]}, + ), + ) + return errors # We need a subject in case domain wide delegation is set, otherwise the # check will fail. This means that the admin will be able to add sheets diff --git a/superset/db_engine_specs/snowflake.py b/superset/db_engine_specs/snowflake.py index c0641ad33344..2fc17e9dffd0 100644 --- a/superset/db_engine_specs/snowflake.py +++ b/superset/db_engine_specs/snowflake.py @@ -34,6 +34,7 @@ from sqlalchemy.engine.reflection import Inspector from sqlalchemy.engine.url import URL from sqlalchemy.exc import DatabaseError as SqlalchemyDatabaseError +from sqlalchemy.sql import quoted_name from sqlalchemy.sql.elements import ColumnElement from superset import is_feature_enabled, security_manager @@ -94,8 +95,9 @@ def __instancecheck__(cls, instance: object) -> bool: if isinstance(instance, SqlalchemyDatabaseError): orig = cast(SqlalchemyDatabaseError, instance).orig - return isinstance(orig, DatabaseError) and "Invalid OAuth access token" in str( - orig + return isinstance(orig, DatabaseError) and ( + getattr(orig, "errno", None) == 390303 + or "Invalid OAuth access token" in str(orig) ) @@ -156,6 +158,17 @@ class SnowflakeEngineSpec(PostgresBaseEngineSpec): supports_catalog = supports_dynamic_catalog = supports_cross_catalog_queries = True supports_grouping_sets = True + @classmethod + def prepare_identifier( + cls, + name: str, + normalize_columns: bool = False, + ) -> str: + """Preserve exact-case physical identifiers when columns are not normalized.""" + if normalize_columns: + return name + return quoted_name(name, quote=True) + metadata = { "description": "Snowflake is a cloud-native data warehouse.", "logo": "snowflake.svg", diff --git a/superset/jinja_context.py b/superset/jinja_context.py index c207196b8077..d7af36094e57 100644 --- a/superset/jinja_context.py +++ b/superset/jinja_context.py @@ -1293,7 +1293,7 @@ def get_dataset_id_from_context(metric_key: str) -> int: """ # pylint: disable=import-outside-toplevel from superset.daos.chart import ChartDAO - from superset.views.utils import loads_request_json + from superset.views.utils import get_request_json_body, loads_request_json form_data: dict[str, Any] = {} exc_message = _( @@ -1302,7 +1302,7 @@ def get_dataset_id_from_context(metric_key: str) -> int: ) if has_request_context(): - if payload := request.get_json(cache=True) if request.is_json else None: + if payload := get_request_json_body(): if dataset_id := payload.get("datasource", {}).get("id"): return dataset_id form_data.update(payload.get("form_data", {})) diff --git a/superset/migrations/versions/2026-08-25_18-00_8f31c5d726ab_index_dataset_dependency_lookups.py b/superset/migrations/versions/2026-08-25_18-00_8f31c5d726ab_index_dataset_dependency_lookups.py new file mode 100644 index 000000000000..46249e1c8a78 --- /dev/null +++ b/superset/migrations/versions/2026-08-25_18-00_8f31c5d726ab_index_dataset_dependency_lookups.py @@ -0,0 +1,51 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Index dataset dependency lookups. + +Revision ID: 8f31c5d726ab +Revises: 39097d124752 +Create Date: 2026-08-25 18:00:00.000000 + +""" + +from superset.migrations.shared.utils import create_index, drop_index + +revision: str = "8f31c5d726ab" +down_revision: str = "39097d124752" + +_SLICE_DATASOURCE_INDEX: str = "ix_slices_datasource_type_datasource_id" +_DASHBOARD_SLICE_INDEX: str = "ix_dashboard_slices_slice_id" + + +def upgrade() -> None: + """Add indexes supporting dataset impact collection.""" + create_index( + "slices", + _SLICE_DATASOURCE_INDEX, + ["datasource_type", "datasource_id"], + ) + create_index( + "dashboard_slices", + _DASHBOARD_SLICE_INDEX, + ["slice_id"], + ) + + +def downgrade() -> None: + """Remove dataset impact lookup indexes.""" + drop_index("dashboard_slices", _DASHBOARD_SLICE_INDEX) + drop_index("slices", _SLICE_DATASOURCE_INDEX) diff --git a/superset/models/dashboard.py b/superset/models/dashboard.py index dbddea06aea3..3d5d4dc59076 100644 --- a/superset/models/dashboard.py +++ b/superset/models/dashboard.py @@ -128,6 +128,7 @@ def copy_dashboard(_mapper: Mapper, _connection: Connection, target: Dashboard) ForeignKey("slices.id", ondelete="CASCADE"), primary_key=True, ), + sqla.Index("ix_dashboard_slices_slice_id", "slice_id"), ) diff --git a/superset/models/helpers.py b/superset/models/helpers.py index 191cc673e7b0..a512e4ab901b 100644 --- a/superset/models/helpers.py +++ b/superset/models/helpers.py @@ -3610,7 +3610,12 @@ def adhoc_metric_to_sqla( aggregate: Any = metric.get("aggregate") metric_column = metric.get("column") or {} column_name = cast(str, metric_column.get("column_name")) - sqla_column = sa.column(column_name) + sqla_column = sa.column( + self.db_engine_spec.prepare_identifier( + column_name, + normalize_columns=bool(self.normalize_columns), + ) + ) if isinstance(aggregate, str) and aggregate in self.sqla_aggregations: sqla_metric = self.sqla_aggregations[aggregate](sqla_column) @@ -4320,7 +4325,11 @@ def convert_tbl_column_to_sqla_col( expression = self._validate_stored_expression(expression) col = literal_column(expression, type_=type_) else: - col = sa.column(tbl_column.column_name, type_=type_) + identifier = db_engine_spec.prepare_identifier( + cast(str, tbl_column.column_name), + normalize_columns=bool(self.normalize_columns), + ) + col = sa.column(identifier, type_=type_) col = self.make_sqla_column_compatible(col, label) return col diff --git a/superset/models/slice.py b/superset/models/slice.py index 8540817ee9fe..5aa09ed6774f 100644 --- a/superset/models/slice.py +++ b/superset/models/slice.py @@ -59,6 +59,12 @@ from superset.common.query_context_factory import QueryContextFactory from superset.connectors.sqla.models import SqlaTable + # avoid circular import: superset.connectors.sqla.models imports this module, + # and superset.semantic_layers.models -> semantic_layers.mapper imports + # superset.connectors.sqla.models. The ``semantic_view`` relationship below + # names its target as a string, so the class is only needed for typing. + from superset.semantic_layers.models import SemanticView + metadata = Model.metadata # pylint: disable=no-member logger = logging.getLogger(__name__) @@ -71,6 +77,13 @@ class Slice( # pylint: disable=too-many-public-methods query_context_factory: QueryContextFactory | None = None __tablename__ = "slices" + __table_args__: tuple[sqla.Index, ...] = ( + sqla.Index( + "ix_slices_datasource_type_datasource_id", + "datasource_type", + "datasource_id", + ), + ) # query_context is excluded: it is a cached/regenerated field, not user-authored. # deleted_at is deletion-state metadata (SoftDeleteMixin), tracked by soft # delete, not content versioning; it is also absent from the slices_version @@ -161,6 +174,24 @@ class Slice( # pylint: disable=too-many-public-methods remote_side="SqlaTable.id", lazy="subquery", ) + # Counterpart of ``table`` for charts built on a semantic view. ``datasource_id`` + # is only unique within a ``datasource_type``, so the join is guarded on the + # type to keep a same-id dataset from being resolved by mistake. View-only: + # ``datasource_id`` is written by the chart, never through this relationship. + # ``selectin`` costs one extra bounded ``IN (...)`` query per batch of charts + # loaded (the type predicate rules out the FK-only shortcut); the default + # lazy load would be one query per semantic-view chart on a list page. The + # string target resolves because ``superset.daos.datasource`` imports + # ``SemanticView`` unconditionally during app initialisation. + semantic_view = relationship( + "SemanticView", + foreign_keys=[datasource_id], + primaryjoin="and_(Slice.datasource_id == SemanticView.id, " + "Slice.datasource_type == 'semantic_view')", + remote_side="SemanticView.id", + viewonly=True, + lazy="selectin", + ) token = "" @@ -186,6 +217,19 @@ def __repr__(self) -> str: def datasource(self) -> SqlaTable | None: return self.table + def _display_datasource(self) -> SqlaTable | SemanticView | None: + """Return the datasource used to name and link this chart in listings. + + Display-only counterpart of ``datasource``: it also resolves semantic + views, selected strictly by ``datasource_type`` so a chart can never be + labelled with a same-id datasource of another kind. ``datasource`` itself + deliberately stays ``SqlaTable``-only because access checks and exports + depend on that type. + """ + if self.datasource_type == utils.DatasourceType.SEMANTIC_VIEW: + return self.semantic_view + return self.table + def clone(self) -> Slice: return Slice( slice_name=self.slice_name, @@ -198,36 +242,26 @@ def clone(self) -> Slice: cache_timeout=self.cache_timeout, ) + # The helpers below read the resolved datasource through ``getattr`` so an + # unresolved reference (``None``) or a datasource kind lacking the attribute + # yields ``None`` for that one chart instead of failing a whole listing. + @renders("datasource_name") def datasource_link(self) -> Markup | None: - datasource = self.datasource - return datasource.link if datasource else None + return getattr(self._display_datasource(), "link", None) @renders("datasource_url") def datasource_url(self) -> str | None: - # Use getattr to guard against datasource types that don't have explore_url - # (e.g. Query objects), which would otherwise raise AttributeError and cause - # the entire chart list response to fail. - if self.table: - return getattr(self.table, "explore_url", None) - datasource = self.datasource - return getattr(datasource, "explore_url", None) if datasource else None + return getattr(self._display_datasource(), "explore_url", None) def datasource_name_text(self) -> str | None: - if self.table: - if self.table.schema: - return f"{self.table.schema}.{self.table.table_name}" - return self.table.table_name - if self.datasource: - if self.datasource.schema: - return f"{self.datasource.schema}.{self.datasource.name}" - return self.datasource.name - return None + # ``SqlaTable.name`` is ``schema.table_name`` when a schema is set; + # ``SemanticView.name`` is the view's name. + return getattr(self._display_datasource(), "name", None) @property def datasource_edit_url(self) -> str | None: - datasource = self.datasource - return datasource.url if datasource else None + return getattr(self._display_datasource(), "url", None) @property def description_markeddown(self) -> str: @@ -368,7 +402,7 @@ def icons(self) -> str: # Escape the data-controlled datasource name and edit URL before they # are interpolated into HTML attributes. url = escape(self.datasource_edit_url) - datasource = escape(self.datasource) + datasource = escape(self.datasource_name_text() or "") return f""" Response: start = page * page_size channels = channels[start : start + page_size] return self.response(200, count=count, result=channels) + except SlackChannelListingClientError as ex: + # Permanent token/client-setup failures are expected, already-handled + # noise (e.g. a revoked bot token), so log at WARNING to keep Sentry + # clear of an actionable-looking signal. + logger.warning("Error fetching slack channels %s", str(ex)) + return self.response_422(message=str(ex)) except SupersetException as ex: + # Transient listing failures (rate limits, transport errors) mean + # Slack is unavailable, so keep ERROR to preserve an actionable signal. logger.error("Error fetching slack channels %s", str(ex)) return self.response_422(message=str(ex)) diff --git a/superset/security/manager.py b/superset/security/manager.py index a12f80cb93fe..36b1262352a2 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -22,6 +22,7 @@ import re import time from collections import defaultdict +from collections.abc import Set as AbstractSet from math import ceil from types import SimpleNamespace from typing import ( @@ -2630,7 +2631,7 @@ def get_schemas_accessible_by_user( self, database: "Database", catalog: Optional[str], - schemas: set[str], + schemas: AbstractSet[str] | list[str], hierarchical: bool = True, ) -> set[str]: """ @@ -2640,7 +2641,7 @@ def get_schemas_accessible_by_user( :param database: The SQL database :param catalog: An optional database catalog - :param schemas: A set of candidate schemas + :param schemas: The candidate schemas :param hierarchical: Whether to check using the hierarchical permission logic :returns: The set of accessible database schemas """ @@ -2648,6 +2649,12 @@ def get_schemas_accessible_by_user( # pylint: disable=import-outside-toplevel from superset.connectors.sqla.models import SqlaTable + # Candidate names may come from cached metadata calls (eg, + # ``Database.get_all_schema_names``) whose values can be deserialized + # as lists rather than sets depending on the cache serializer, so + # normalize before applying set operations. + schemas = set(schemas) + default_catalog = database.get_default_catalog() catalog = catalog or default_catalog @@ -2700,20 +2707,26 @@ def get_schemas_accessible_by_user( def get_catalogs_accessible_by_user( self, database: "Database", - catalogs: set[str], + catalogs: AbstractSet[str] | list[str], hierarchical: bool = True, ) -> set[str]: """ Returned a filtered list of the catalogs accessible by the user. :param database: The SQL database - :param catalogs: A set of candidate catalogs + :param catalogs: The candidate catalogs :param hierarchical: Whether to check using the hierarchical permission logic :returns: The set of accessible database catalogs """ # pylint: disable=import-outside-toplevel from superset.connectors.sqla.models import SqlaTable + # Candidate names may come from cached metadata calls (eg, + # ``Database.get_all_catalog_names``) whose values can be deserialized + # as lists rather than sets depending on the cache serializer, so + # normalize before applying set operations. + catalogs = set(catalogs) + if hierarchical and self.can_access_database(database): return catalogs diff --git a/superset/sql/execution/celery_task.py b/superset/sql/execution/celery_task.py index 619725563c01..7b56db8e4cc6 100644 --- a/superset/sql/execution/celery_task.py +++ b/superset/sql/execution/celery_task.py @@ -350,10 +350,17 @@ def execute_sql_task( with app.test_request_context(): with override_user(security_manager.find_user(username)): try: - return _execute_sql_statements( - query_id, - rendered_query, - start_time=start_time, + from superset.utils.oauth2 import execute_with_oauth2_retry + + query = _get_query(query_id=query_id) + return execute_with_oauth2_retry( + query.database, + lambda: _execute_sql_statements( + query_id, + rendered_query, + start_time=start_time, + ), + can_retry=lambda: not query.progress, ) except Exception as ex: logger.exception("Query %d: %s", query_id, ex) diff --git a/superset/sql/execution/executor.py b/superset/sql/execution/executor.py index 532699a74aa1..07fd64c33f16 100644 --- a/superset/sql/execution/executor.py +++ b/superset/sql/execution/executor.py @@ -647,49 +647,57 @@ def _execute_statements( results_list = [] - # Use consistent execution path for all queries - with self.database.get_raw_connection(catalog=catalog, schema=schema) as conn: - with contextlib.closing(conn.cursor()) as cursor: - execution_results = execute_sql_with_cursor( - database=self.database, - cursor=cursor, - statements=[ - stmt.format() for stmt in transformed_script.statements - ], - query=query, - log_query_fn=self._log_query, - ) + def execute() -> list[tuple[str, SupersetResultSet | None, float, int]]: + with self.database.get_raw_connection( + catalog=catalog, schema=schema + ) as conn: + with contextlib.closing(conn.cursor()) as cursor: + return execute_sql_with_cursor( + database=self.database, + cursor=cursor, + statements=[ + stmt.format() for stmt in transformed_script.statements + ], + query=query, + log_query_fn=self._log_query, + ) - # If execution was stopped or returned no results, return early - if not execution_results: - return [] - - # Build StatementResult for each executed statement - # with both original and executed SQL - for orig_sql, (exec_sql, result_set, exec_time, rowcount) in zip( - original_sqls, execution_results, strict=True - ): - if result_set is not None: - # SELECT statement - df = result_set.to_pandas_df() - stmt_result = StatementResult( - original_sql=orig_sql, - executed_sql=exec_sql, - data=df, - row_count=len(df), - execution_time_ms=exec_time, - ) - else: - # DML statement - no data, just row count - stmt_result = StatementResult( - original_sql=orig_sql, - executed_sql=exec_sql, - data=None, - row_count=rowcount, - execution_time_ms=exec_time, - ) + from superset.utils.oauth2 import execute_with_oauth2_retry + + execution_results = execute_with_oauth2_retry( + self.database, execute, can_retry=lambda: not query.progress + ) + + # If execution was stopped or returned no results, return early + if not execution_results: + return [] + + # Build StatementResult for each executed statement + # with both original and executed SQL + for orig_sql, (exec_sql, result_set, exec_time, rowcount) in zip( + original_sqls, execution_results, strict=True + ): + if result_set is not None: + # SELECT statement + df = result_set.to_pandas_df() + stmt_result = StatementResult( + original_sql=orig_sql, + executed_sql=exec_sql, + data=df, + row_count=len(df), + execution_time_ms=exec_time, + ) + else: + # DML statement - no data, just row count + stmt_result = StatementResult( + original_sql=orig_sql, + executed_sql=exec_sql, + data=None, + row_count=rowcount, + execution_time_ms=exec_time, + ) - results_list.append(stmt_result) + results_list.append(stmt_result) return results_list diff --git a/superset/translations/messages.pot b/superset/translations/messages.pot index cd26a4759b73..b17e5c617d45 100644 --- a/superset/translations/messages.pot +++ b/superset/translations/messages.pot @@ -17246,3 +17246,36 @@ msgstr "" msgid "№" msgstr "" + +msgid "(archived)" +msgstr "" + +#, python-format +msgid "%(count)s %(label)s" +msgstr "" + +#, python-format +msgid "%(count)s additional restricted %(label)s" +msgstr "" + +msgid "Checking charts and dashboards that use this dataset…" +msgstr "" + +msgid "Deleting this dataset is permanent. The affected charts and dashboards will remain, but they may no longer work." +msgstr "" + +msgid "The affected charts or dashboards changed. Review the updated impact and type DELETE again to continue." +msgstr "" + +msgid "The deletion impact could not be determined. This dataset cannot be permanently deleted until the check succeeds." +msgstr "" + +#, python-format +msgid "No affected %(label)s." +msgstr "" + +msgid "Show fewer" +msgstr "" + +msgid "Show all" +msgstr "" diff --git a/superset/utils/oauth2.py b/superset/utils/oauth2.py index 071f54fd3714..36cbf8250a6e 100644 --- a/superset/utils/oauth2.py +++ b/superset/utils/oauth2.py @@ -22,13 +22,15 @@ import logging import secrets from contextlib import contextmanager +from contextvars import ContextVar from datetime import datetime, timedelta, timezone -from typing import Any, Iterator, TYPE_CHECKING +from typing import Any, Callable, Iterator, TYPE_CHECKING, TypeVar import backoff import jwt -from flask import current_app as app, url_for +from flask import current_app as app, g, url_for from marshmallow import EXCLUDE, fields, post_load, Schema, validate +from sqlalchemy.orm import Session from werkzeug.routing import BuildError from superset import db @@ -47,6 +49,10 @@ JWT_EXPIRATION = timedelta(minutes=5) logger = logging.getLogger(__name__) +T = TypeVar("T") +_oauth2_retry_active: ContextVar[bool] = ContextVar( + "oauth2_retry_active", default=False +) # PKCE code verifier length (RFC 7636 recommends 43-128 characters) PKCE_CODE_VERIFIER_LENGTH = 64 @@ -128,12 +134,44 @@ def get_oauth2_access_token( return None -def refresh_oauth2_token( +def refresh_oauth2_token( # noqa: C901 + config: OAuth2ClientConfig, + database_id: int, + user_id: int, + db_engine_spec: type[BaseEngineSpec], + *, + force: bool = False, + rejected_access_token: str | None = None, +) -> str | None: + # Forced refreshes use an isolated transaction so rotated tokens become durable + # without committing unrelated work in the caller's scoped session. + token_session = Session(bind=db.session.get_bind()) if force else db.session + try: + return _refresh_oauth2_token_locked( + config, + database_id, + user_id, + db_engine_spec, + token_session, + force=force, + rejected_access_token=rejected_access_token, + ) + finally: + if force: + token_session.close() + + +def _refresh_oauth2_token_locked( # noqa: C901 config: OAuth2ClientConfig, database_id: int, user_id: int, db_engine_spec: type[BaseEngineSpec], + token_session: Session, + *, + force: bool, + rejected_access_token: str | None, ) -> str | None: + """Refresh a token while serializing and durably persisting the exchange.""" # pylint: disable=import-outside-toplevel from superset.models.core import DatabaseUserOAuth2Tokens @@ -145,19 +183,34 @@ def refresh_oauth2_token( database_id=database_id, ): # Short circuit in case another request already deleted the token - token = ( - db.session.query(DatabaseUserOAuth2Tokens) - .filter_by(user_id=user_id, database_id=database_id) - .one_or_none() - ) + query = token_session.query(DatabaseUserOAuth2Tokens) + if force: + query = query.populate_existing() + token = query.filter_by(user_id=user_id, database_id=database_id).one_or_none() if token is None: return None - if token.access_token and datetime.now() < token.access_token_expiration: + # Another request may have refreshed the token while this caller waited + # for the distributed lock. Reuse the winner rather than exchanging a + # rotating/single-use refresh token again. + if ( + force + and rejected_access_token is not None + and token.access_token != rejected_access_token + ): + return token.access_token + + if ( + not force + and token.access_token + and datetime.now() < token.access_token_expiration + ): return token.access_token if not token.refresh_token: - db.session.delete(token) + token_session.delete(token) + if force: + token_session.commit() # pylint: disable=consider-using-transaction return None try: @@ -174,8 +227,10 @@ def refresh_oauth2_token( db_engine_spec.engine, type(ex).__name__, ) - db.session.delete(token) - db.session.flush() + token_session.delete(token) + token_session.flush() + if force: + token_session.commit() # pylint: disable=consider-using-transaction raise OAuth2TokenRefreshError() from None # Engine specs can delegate to arbitrary provider clients that do not share an # exception base class. Sanitize every other provider-boundary failure while @@ -202,11 +257,111 @@ def refresh_oauth2_token( if new_refresh_token := token_response.get("refresh_token"): token.refresh_token = new_refresh_token - db.session.add(token) + token_session.add(token) + if force: + # Make rotated access and refresh tokens visible to other workers before + # releasing the distributed lock. Query execution already commits its + # audit/progress state, so this does not introduce a new transaction + # boundary for the query paths using forced refresh. + token_session.commit() # pylint: disable=consider-using-transaction return token.access_token +def execute_with_oauth2_retry( # noqa: C901 + database: Database, + operation: Callable[[], T], + can_retry: Callable[[], bool] | None = None, +) -> T: + """Refresh a rejected access token and retry an operation once.""" + # pylint: disable=import-outside-toplevel + from superset.models.core import DatabaseUserOAuth2Tokens + + user = getattr(g, "user", None) + user_id = getattr(user, "id", None) + rejected_access_token = None + if user_id is not None: + with db.session.no_autoflush: + token = ( + db.session.query(DatabaseUserOAuth2Tokens) + .filter_by(user_id=user_id, database_id=database.id) + .one_or_none() + ) + rejected_access_token = token.access_token if token is not None else None + + retry_context = _oauth2_retry_active.set(True) + try: + try: + return operation() + finally: + _oauth2_retry_active.reset(retry_context) + except Exception as ex: + is_oauth2_error = ( + database.is_oauth2_enabled() and database.db_engine_spec.needs_oauth2(ex) + ) + if not is_oauth2_error: + raise + if can_retry is not None and not can_retry(): + app.config["STATS_LOGGER"].incr( + "oauth2.forced_refresh.query_retry_skipped_progress" + ) + database.start_oauth2_dance() + raise + + config = database.get_oauth2_config() + if config is None or user_id is None: + app.config["STATS_LOGGER"].incr("oauth2.forced_refresh.unavailable") + raise + + stats_logger = app.config["STATS_LOGGER"] + stats_logger.incr("oauth2.forced_refresh.exchange_attempt") + logger.info( + "Forcing OAuth2 token refresh after authentication failure: " + "database_id=%s engine=%s", + database.id, + database.db_engine_spec.engine, + ) + try: + access_token = refresh_oauth2_token( + config, + database.id, + user_id, + database.db_engine_spec, + force=True, + rejected_access_token=rejected_access_token, + ) + except OAuth2TokenRefreshError: + stats_logger.incr("oauth2.forced_refresh.exchange_rejected") + database.start_oauth2_dance() + raise + except Exception: + stats_logger.incr("oauth2.forced_refresh.exchange_transient_failure") + raise + + if access_token is None: + stats_logger.incr("oauth2.forced_refresh.unavailable") + database.start_oauth2_dance() + + # The forced refresh commits through an isolated session. Expire the token + # loaded above so connection creation for the retry observes that commit. + if token is not None: + db.session.expire(token) + + stats_logger.incr("oauth2.forced_refresh.exchange_success") + try: + result = operation() + except Exception: + stats_logger.incr("oauth2.forced_refresh.query_retry_failure") + raise + stats_logger.incr("oauth2.forced_refresh.query_retry_success") + return result + + +def is_oauth2_retry_active() -> bool: + """Return whether an outer query execution can retry an OAuth2 failure.""" + return _oauth2_retry_active.get() + + def encode_oauth2_state(state: OAuth2State) -> str: """ Encode the OAuth2 state. @@ -322,9 +477,13 @@ def check_for_oauth2(database: Database) -> Iterator[None]: try: yield except Exception as ex: - if database.is_oauth2_enabled() and ( - isinstance(ex, OAuth2TokenRefreshError) - or database.db_engine_spec.needs_oauth2(ex) + if ( + not is_oauth2_retry_active() + and database.is_oauth2_enabled() + and ( + isinstance(ex, OAuth2TokenRefreshError) + or database.db_engine_spec.needs_oauth2(ex) + ) ): database.db_engine_spec.start_oauth2_dance(database) raise diff --git a/superset/utils/slack.py b/superset/utils/slack.py index aa4b5c12db36..2d3ca73c8f56 100644 --- a/superset/utils/slack.py +++ b/superset/utils/slack.py @@ -141,6 +141,10 @@ class SlackChannelCacheWriteError(SupersetException): } ) +_AUTH_ERROR_CODES = frozenset( + {"not_authed", "invalid_auth", "account_inactive", "token_revoked", "token_expired"} +) + SLACK_TRANSIENT_TRANSPORT_ERRORS: tuple[type[Exception], ...] = ( SlackClientNotConnectedError, URLError, @@ -392,12 +396,24 @@ def _get_channels(team_id: Optional[str] = None) -> list[SlackChannel]: ) return channels except SlackApiError as ex: - logger.error( - "Failed to fetch Slack channels after %d pages: %s", - page_count, - str(ex), - exc_info=True, - ) + # Only bot-token auth failures (invalid/revoked/deactivated) are the + # expected, already-handled multi-tenant condition this is meant to + # quiet down. Rate limits and Slack server/API errors are actionable + # outages, so they keep ERROR-level logging with a traceback. + error_code = get_slack_api_error_code(ex) + if error_code in _AUTH_ERROR_CODES: + logger.warning( + "Failed to fetch Slack channels after %d pages: %s", + page_count, + str(ex), + ) + else: + logger.error( + "Failed to fetch Slack channels after %d pages: %s", + page_count, + str(ex), + exc_info=True, + ) raise diff --git a/tests/integration_tests/datasets/soft_delete_tests.py b/tests/integration_tests/datasets/soft_delete_tests.py index 000515e3d28d..57d43e4b91e6 100644 --- a/tests/integration_tests/datasets/soft_delete_tests.py +++ b/tests/integration_tests/datasets/soft_delete_tests.py @@ -17,7 +17,10 @@ """Integration tests for dataset soft-delete and restore.""" from datetime import datetime +from typing import cast +from unittest.mock import patch +from flask import Response from flask_appbuilder.security.sqla.models import User from superset import security_manager @@ -755,7 +758,28 @@ def test_purge_by_owner_permanently_deletes(self) -> None: db.session.commit() self.login(ADMIN_USERNAME) - rv = self.client.post(f"/api/v1/dataset/{dataset_uuid}/purge") + impact_rv: Response = self.client.get( + f"/api/v1/dataset/{dataset_uuid}/purge-impact" + ) + assert impact_rv.status_code == 200, impact_rv.data + impact: dict[str, object] = json.loads(impact_rv.data) + assert impact["charts"] == { + "count": 0, + "restricted_count": 0, + "result": [], + } + assert impact["dashboards"] == { + "count": 0, + "restricted_count": 0, + "result": [], + } + impact_token: str = str(impact["impact_token"]) + assert impact_token.startswith("v1:") + + rv: Response = self.client.post( + f"/api/v1/dataset/{dataset_uuid}/purge", + json={"confirmed_impact_token": impact_token}, + ) assert rv.status_code == 200, rv.data row = ( @@ -770,6 +794,141 @@ def test_purge_by_owner_permanently_deletes(self) -> None: db.session.delete(database) db.session.commit() + @with_feature_flags(SOFT_DELETE=True) + def test_dataset_purge_rejects_missing_confirmation_token(self) -> None: + """A malformed client request cannot bypass impact confirmation.""" + created: tuple[SqlaTable, Database] = self._make("arch_purge_missing_token") + dataset: SqlaTable = created[0] + database: Database = created[1] + dataset_id: int = dataset.id + dataset_uuid: str = str(dataset.uuid) + dataset.deleted_at = datetime(2026, 1, 1, 12, 0, 0) + db.session.commit() + + self.login(ADMIN_USERNAME) + rv: Response = self.client.post( + f"/api/v1/dataset/{dataset_uuid}/purge", json={} + ) + + assert rv.status_code == 400, rv.data + assert ( + db.session.query(SqlaTable) + .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {SqlaTable}}) + .filter(SqlaTable.id == dataset_id) + .one_or_none() + is not None + ) + self._cleanup(dataset_id, database) + + @with_feature_flags(SOFT_DELETE=True) + def test_dataset_purge_requires_reconfirmation_when_impact_changes(self) -> None: + """A stale token returns refreshed impact without mutating the dataset.""" + created: tuple[SqlaTable, Database] = self._make("arch_purge_stale_token") + dataset: SqlaTable = created[0] + database: Database = created[1] + dataset_id: int = dataset.id + dataset_uuid: str = str(dataset.uuid) + dataset.deleted_at = datetime(2026, 1, 1, 12, 0, 0) + db.session.commit() + + self.login(ADMIN_USERNAME) + preview_rv: Response = self.client.get( + f"/api/v1/dataset/{dataset_uuid}/purge-impact" + ) + preview: dict[str, object] = json.loads(preview_rv.data) + stale_token: str = str(preview["impact_token"]) + + chart: Slice = Slice( + slice_name="new impact chart", + datasource_id=dataset_id, + datasource_type="table", + viz_type="table", + ) + db.session.add(chart) + db.session.commit() + + stale_rv: Response = self.client.post( + f"/api/v1/dataset/{dataset_uuid}/purge", + json={"confirmed_impact_token": stale_token}, + ) + stale_payload: dict[str, object] = json.loads(stale_rv.data) + + assert stale_rv.status_code == 409, stale_rv.data + assert stale_payload["reason"] == "purge_impact_changed" + refreshed: dict[str, object] = cast(dict[str, object], stale_payload["impact"]) + refreshed_charts: dict[str, object] = cast( + dict[str, object], refreshed["charts"] + ) + assert refreshed_charts["count"] == 1 + assert db.session.get(SqlaTable, dataset_id) is not None + assert db.session.get(Slice, chart.id) is not None + + refreshed_token: str = str(refreshed["impact_token"]) + confirmed_rv: Response = self.client.post( + f"/api/v1/dataset/{dataset_uuid}/purge", + json={"confirmed_impact_token": refreshed_token}, + ) + assert confirmed_rv.status_code == 200, confirmed_rv.data + assert db.session.get(SqlaTable, dataset_id) is None + + db.session.delete(chart) + db.session.delete(database) + db.session.commit() + + @with_feature_flags(SOFT_DELETE=True) + def test_purge_impact_redacts_restricted_chart_for_non_admin_editor( + self, + ) -> None: + """Dataset editorship exposes impact cardinality, not object details.""" + alpha: User = self.get_user(ALPHA_USERNAME) + database: Database = Database( + database_name="db_arch_impact_redaction", sqlalchemy_uri="sqlite://" + ) + db.session.add(database) + db.session.flush() + dataset: SqlaTable = SqlaTable( + table_name="arch_impact_redaction", + database=database, + editors=[_user_subject(alpha)], + ) + db.session.add(dataset) + db.session.flush() + chart: Slice = Slice( + slice_name="restricted impact chart", + datasource_id=dataset.id, + datasource_type="table", + viz_type="table", + ) + db.session.add(chart) + db.session.commit() + dataset_id: int = dataset.id + dataset_uuid: str = str(dataset.uuid) + chart_uuid: str = str(chart.uuid) + dataset.deleted_at = datetime(2026, 1, 1, 12, 0, 0) + db.session.commit() + + try: + self.login(ALPHA_USERNAME) + with patch.object(security_manager, "can_access_chart", return_value=False): + rv: Response = self.client.get( + f"/api/v1/dataset/{dataset_uuid}/purge-impact" + ) + + assert rv.status_code == 200, rv.data + payload: dict[str, object] = json.loads(rv.data) + assert payload["charts"] == { + "count": 1, + "restricted_count": 1, + "result": [], + } + serialized_payload: str = rv.data.decode() + assert chart_uuid not in serialized_payload + assert "restricted impact chart" not in serialized_payload + finally: + db.session.delete(chart) + db.session.commit() + self._cleanup(dataset_id, database) + @with_feature_flags(SOFT_DELETE=True) def test_purge_blocked_for_non_owner(self) -> None: """A non-owner (Gamma) cannot permanently delete another user's archived diff --git a/tests/integration_tests/datasource/api_tests.py b/tests/integration_tests/datasource/api_tests.py index 241fc7faed4e..454f91ce57e7 100644 --- a/tests/integration_tests/datasource/api_tests.py +++ b/tests/integration_tests/datasource/api_tests.py @@ -15,21 +15,101 @@ # specific language governing permissions and limitations # under the License. +import uuid +from collections.abc import Iterator +from contextlib import contextmanager from datetime import datetime from unittest.mock import ANY, patch import pytest +from flask_appbuilder.security.sqla.models import PermissionView from sqlalchemy.sql.elements import TextClause from superset import db, security_manager from superset.connectors.sqla.models import SqlaTable from superset.daos.exceptions import DatasourceTypeNotSupportedError from superset.extensions import cache_manager +from superset.semantic_layers.models import SemanticLayer, SemanticView from superset.utils import json from tests.integration_tests.base_tests import SupersetTestCase +from tests.integration_tests.conftest import with_feature_flags from tests.integration_tests.constants import ADMIN_USERNAME, GAMMA_USERNAME +@contextmanager +def _semantic_views(*layer_names: str) -> Iterator[list[SemanticView]]: + """Persist one semantic view per named layer and remove them on exit. + + Names are suffixed to stay unique across runs. The layer and view insert + hooks create the matching ``datasource_access`` permissions; the delete + hooks remove them again together with any role associations. + """ + suffix = uuid.uuid4().hex + layers = [ + SemanticLayer( + uuid=uuid.uuid4(), + name=f"{name}_{suffix}", + type="test", + configuration="{}", + ) + for name in layer_names + ] + views = [ + SemanticView( + uuid=uuid.uuid4(), + name=f"{layer.name}_view", + semantic_layer_uuid=layer.uuid, + configuration="{}", + ) + for layer in layers + ] + db.session.add_all([*layers, *views]) + db.session.commit() + try: + for obj in [*layers, *views]: + db.session.refresh(obj) + yield views + finally: + db.session.rollback() + for obj in [*views, *layers]: + db.session.delete(obj) + db.session.commit() + + +@contextmanager +def _gamma_granted(*pvms: tuple[str, str]) -> Iterator[None]: + """Grant ``(permission, view_menu)`` pairs to Gamma for the block's duration. + + A permission-view that does not exist yet (``can_read SemanticView`` is only + registered when the semantic view API is mounted) is created and removed + again afterwards; one that already exists is left in place. + """ + gamma = security_manager.find_role("Gamma") + assert gamma is not None + created: list[PermissionView] = [] + granted: list[PermissionView] = [] + for permission, view_menu in pvms: + pvm = security_manager.find_permission_view_menu(permission, view_menu) + if pvm is None: + pvm = security_manager.add_permission_view_menu(permission, view_menu) + created.append(pvm) + if pvm not in gamma.permissions: + security_manager.add_permission_role(gamma, pvm) + granted.append(pvm) + db.session.commit() + try: + yield + finally: + db.session.rollback() + for pvm in granted: + security_manager.del_permission_role(gamma, pvm) + for pvm in created: + security_manager.del_permission_view_menu( + pvm.permission.name, pvm.view_menu.name + ) + db.session.commit() + + class TestDatasourceApi(SupersetTestCase): def setUp(self): # Clear the column-values cache before every test so that @@ -477,3 +557,74 @@ def test_combined_list_gamma_uses_read_permission(self, run_mock): response = json.loads(rv.data.decode("utf-8")) assert response == {"count": 0, "result": []} run_mock.assert_called_once() + + def _list_semantic_views_as_gamma(self) -> tuple[int, set[str]]: + """Fetch the semantic-view slice of the combined list as Gamma.""" + self.login(GAMMA_USERNAME) + rv = self.client.get( + "api/v1/datasource/?q=" + "(filters:!((col:source_type,opr:eq,value:semantic_layer))," + "order_column:table_name,order_direction:asc,page:0,page_size:25)" + ) + payload = json.loads(rv.data.decode("utf-8")) + names = {item["table_name"] for item in payload.get("result", [])} + return rv.status_code, names + + @with_feature_flags(SEMANTIC_LAYERS=True) + def test_combined_list_gamma_sees_views_of_granted_layers(self): + """A layer-level grant exposes that layer's views and no others.""" + with _semantic_views("permitted", "denied") as (permitted, denied): + layer_perm = permitted.semantic_layer.perm + assert layer_perm is not None + # The layer insert hook creates this permission; a regression + # there must fail here rather than be papered over by the grant. + assert ( + security_manager.find_permission_view_menu( + "datasource_access", layer_perm + ) + is not None + ) + with _gamma_granted( + ("can_read", "SemanticView"), ("datasource_access", layer_perm) + ): + status, names = self._list_semantic_views_as_gamma() + assert status == 200 + assert permitted.name in names + assert denied.name not in names + + @with_feature_flags(SEMANTIC_LAYERS=True) + def test_combined_list_gamma_sees_individually_granted_views(self): + """A view-level grant exposes that view without touching other layers.""" + with _semantic_views("granted", "other") as (granted, other): + view_perm = granted.perm + assert view_perm is not None + assert ( + security_manager.find_permission_view_menu( + "datasource_access", view_perm + ) + is not None + ) + with _gamma_granted( + ("can_read", "SemanticView"), ("datasource_access", view_perm) + ): + status, names = self._list_semantic_views_as_gamma() + assert status == 200 + assert granted.name in names + assert other.name not in names + + @with_feature_flags(SEMANTIC_LAYERS=True) + def test_combined_list_gamma_without_semantic_view_read_gets_none(self): + """Without can_read on SemanticView the semantic-layer slice is empty.""" + gamma = security_manager.find_role("Gamma") + assert gamma is not None + read_pvm = security_manager.find_permission_view_menu( + "can_read", "SemanticView" + ) + assert read_pvm is None or read_pvm not in gamma.permissions + with _semantic_views("layer") as (view,): + layer_perm = view.semantic_layer.perm + assert layer_perm is not None + with _gamma_granted(("datasource_access", layer_perm)): + status, names = self._list_semantic_views_as_gamma() + assert status == 200 + assert names == set() diff --git a/tests/integration_tests/deletion_retention/force_purge_tests.py b/tests/integration_tests/deletion_retention/force_purge_tests.py index 26afa72db609..104dec24d560 100644 --- a/tests/integration_tests/deletion_retention/force_purge_tests.py +++ b/tests/integration_tests/deletion_retention/force_purge_tests.py @@ -30,6 +30,10 @@ AmbiguousPurgeTargetError, ForcePurgeCommand, ) +from superset.commands.deletion_retention.purge_impact import ( + collect_dataset_purge_impact, + PurgeImpactChangedError, +) from superset.connectors.sqla.models import SqlaTable from superset.models.dashboard import Dashboard from superset.models.slice import Slice @@ -102,6 +106,35 @@ def test_force_purge_dataset_leaves_chart_dangling(self) -> None: assert audit.affected_referrers assert chart_uuid in audit.affected_referrers + def test_dataset_impact_drift_fails_audit_without_mutation(self) -> None: + """The locked recheck records a failed no-op when impact changed.""" + reviewed_chart: Slice = self.make_chart("impact_reviewed", dataset=self.dataset) + dataset_id: int = self.dataset.id + dataset_uuid: str = str(self.dataset.uuid) + reviewed_token: str = collect_dataset_purge_impact( + db.session, dataset_id + ).impact_token + changed_chart: Slice = self.make_chart("impact_changed", dataset=self.dataset) + reviewed_chart_id: int = reviewed_chart.id + changed_chart_id: int = changed_chart.id + + with pytest.raises(PurgeImpactChangedError): + ForcePurgeCommand( + dataset_uuid, + model_cls=SqlaTable, + confirmed_impact_token=reviewed_token, + ).run() + + assert self.exists(SqlaTable, dataset_id) + assert self.exists(Slice, reviewed_chart_id) + assert self.exists(Slice, changed_chart_id) + audit_row: PurgeAuditLog = ( + db.session.query(PurgeAuditLog).filter_by(entity_uuid=dataset_uuid).one() + ) + assert audit_row.status == audit.STATUS_FAILED + assert audit_row.removed_dashboard_slices == 0 + assert audit_row.affected_referrers is None + def test_force_purge_counts_removed_dashboard_slices_before_db_cascade( self, ) -> None: diff --git a/tests/unit_tests/charts/test_chart_list_datasource.py b/tests/unit_tests/charts/test_chart_list_datasource.py new file mode 100644 index 000000000000..a53731a09443 --- /dev/null +++ b/tests/unit_tests/charts/test_chart_list_datasource.py @@ -0,0 +1,144 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Chart listings must name and link the semantic view behind a chart. + +A chart's ``datasource_id`` is only unique within its ``datasource_type``, so a +semantic view and a regular dataset routinely share a numeric id. These tests +use real rows with a deliberate id collision to pin that a chart always resolves +to a datasource of its own kind. They drive the ORM relationship directly: the +chart list API serialises these same model helpers (``datasource_name_text``, +``datasource_url``) without transformation. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import pytest + +from superset.connectors.sqla.models import SqlaTable +from superset.models.core import Database +from superset.models.slice import Slice +from superset.semantic_layers.models import SemanticLayer, SemanticView + +if TYPE_CHECKING: + from sqlalchemy.orm import Session + +SHARED_ID = 7 + +# The resolver does not consult the feature flag; this pins that charts already +# built on a semantic view keep resolving after SEMANTIC_LAYERS is turned off. +FLAG_OFF = pytest.mark.parametrize( + "app", + [{"FEATURE_FLAGS": {"SEMANTIC_LAYERS": False}}], + ids=["semantic_layers_off"], + indirect=True, +) + + +def _seed(session: Session) -> tuple[Slice, Slice]: + """Insert a semantic view and a dataset sharing an id, and a chart on each.""" + SqlaTable.metadata.create_all(session.get_bind()) # pylint: disable=no-member + database = Database(database_name="db", sqlalchemy_uri="sqlite://") + table = SqlaTable( + id=SHARED_ID, table_name="aaa_table", schema="public", database=database + ) + layer = SemanticLayer(name="demo", type="demo", configuration={}) + view = SemanticView( + id=SHARED_ID, name="orders", semantic_layer=layer, configuration={} + ) + view_chart = Slice( + slice_name="On the view", + datasource_type="semantic_view", + datasource_id=SHARED_ID, + datasource_name="orders", + viz_type="table", + params="{}", + ) + table_chart = Slice( + slice_name="On the table", + datasource_type="table", + datasource_id=SHARED_ID, + datasource_name="aaa_table", + viz_type="table", + params="{}", + ) + session.add_all([database, table, layer, view, view_chart, table_chart]) + session.flush() + session.expire_all() + return view_chart, table_chart + + +@FLAG_OFF +def test_semantic_view_chart_resolves_to_the_view_not_the_colliding_table( + app: object, + session: Session, +) -> None: + """The display name/link come from the semantic view sharing the id.""" + view_chart, table_chart = _seed(session) + + assert view_chart.datasource_name_text() == "orders" + assert ( + view_chart.datasource_url() + == f"/explore/?datasource_type=semantic_view&datasource_id={SHARED_ID}" + ) + assert ( + view_chart.datasource_edit_url + == f"/semantic_view/{view_chart.semantic_view.uuid}/" + ) + # No legacy HTML renderer exists for semantic views; the helper degrades. + assert view_chart.datasource_link() is None + # The dataset chart on the same numeric id is untouched by the new path. + assert table_chart.datasource_name_text() == "public.aaa_table" + assert ( + table_chart.datasource_url() + == f"/explore/?datasource_type=table&datasource_id={SHARED_ID}" + ) + + +def test_semantic_view_chart_with_deleted_view_degrades_to_empty( + session: Session, +) -> None: + """A dangling semantic-view id yields empty fields, never an error.""" + view_chart, _ = _seed(session) + session.delete(session.get(SemanticView, SHARED_ID)) + session.flush() + session.expire_all() + + assert view_chart.datasource_name_text() is None + assert view_chart.datasource_url() is None + assert view_chart.datasource_edit_url is None + assert view_chart.datasource_link() is None + + +def test_charts_sort_by_stored_datasource_name_across_kinds( + session: Session, +) -> None: + """Characterisation: list sorting uses the stored ``datasource_name`` column. + + The chart list orders by the name captured when the chart was saved, not by + the resolved display name, for datasets and semantic views alike; the two + can disagree after a rename. Pinned here so the split is deliberate. + """ + view_chart, table_chart = _seed(session) + + ordered = [ + slc.id for slc in session.query(Slice).order_by(Slice.datasource_name).all() + ] + + assert ordered.index(table_chart.id) < ordered.index(view_chart.id) + assert view_chart.datasource_name == "orders" diff --git a/tests/unit_tests/commands/deletion_retention/test_purge_impact.py b/tests/unit_tests/commands/deletion_retention/test_purge_impact.py new file mode 100644 index 000000000000..5411ea22b9f6 --- /dev/null +++ b/tests/unit_tests/commands/deletion_retention/test_purge_impact.py @@ -0,0 +1,99 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +"""Tests for authoritative dataset purge-impact snapshots.""" + +from __future__ import annotations + +from contextlib import nullcontext +from datetime import datetime, timezone +from unittest.mock import MagicMock, patch +from uuid import UUID + +from sqlalchemy.orm import Session + +from superset.commands.deletion_retention.purge_impact import ( + _impact_token, + collect_dataset_purge_impact, + DatasetImpactObject, + DatasetPurgeImpact, +) + + +def _object(kind_id: int, uuid: str, *, archived: bool = False) -> DatasetImpactObject: + return DatasetImpactObject( + id=kind_id, + uuid=uuid, + name=f"object-{kind_id}", + archived=archived, + ) + + +def test_impact_token_canonicalizes_uuid_order() -> None: + """Database ordering must not be part of the confirmation contract.""" + first: DatasetImpactObject = _object(1, "00000000-0000-0000-0000-000000000001") + second: DatasetImpactObject = _object(2, "00000000-0000-0000-0000-000000000002") + + forward: str = _impact_token((first, second), ()) + reverse: str = _impact_token((second, first), ()) + + assert forward == reverse + assert forward.startswith("v1:") + + +def test_impact_token_namespaces_chart_and_dashboard_identities() -> None: + """Moving the same UUID between object kinds changes the reviewed impact.""" + shared: DatasetImpactObject = _object(1, "00000000-0000-0000-0000-000000000001") + + assert _impact_token((shared,), ()) != _impact_token((), (shared,)) + + +def test_impact_token_detects_same_count_identity_substitution() -> None: + original: DatasetImpactObject = _object(1, "00000000-0000-0000-0000-000000000001") + replacement: DatasetImpactObject = _object( + 2, "00000000-0000-0000-0000-000000000002" + ) + + assert _impact_token((original,), ()) != _impact_token((replacement,), ()) + + +def test_collector_includes_archived_objects_and_deduplicates_dashboards() -> None: + archived_at: datetime = datetime(2026, 8, 1, tzinfo=timezone.utc) + chart_uuid: UUID = UUID("00000000-0000-0000-0000-000000000001") + dashboard_uuid: UUID = UUID("00000000-0000-0000-0000-000000000002") + chart_result: MagicMock = MagicMock() + chart_result.tuples.return_value = [(10, chart_uuid, "Archived chart", archived_at)] + dashboard_result: MagicMock = MagicMock() + # A dashboard containing two affected charts is one impacted dashboard. + dashboard_result.tuples.return_value = [ + (20, dashboard_uuid, "Archived dashboard", archived_at), + (20, dashboard_uuid, "Archived dashboard", archived_at), + ] + session: MagicMock = MagicMock(spec=Session) + session.execute.side_effect = [chart_result, dashboard_result] + + with patch( + "superset.commands.deletion_retention.purge_impact.skip_visibility_filter", + return_value=nullcontext(), + ): + impact: DatasetPurgeImpact = collect_dataset_purge_impact(session, dataset_id=7) + + assert [(item.uuid, item.archived) for item in impact.charts] == [ + (str(chart_uuid), True) + ] + assert [(item.uuid, item.archived) for item in impact.dashboards] == [ + (str(dashboard_uuid), True) + ] diff --git a/tests/unit_tests/connectors/sqla/models_test.py b/tests/unit_tests/connectors/sqla/models_test.py index 55c021a7c16d..44f68c0b36d7 100644 --- a/tests/unit_tests/connectors/sqla/models_test.py +++ b/tests/unit_tests/connectors/sqla/models_test.py @@ -21,6 +21,7 @@ import pytest from pytest_mock import MockerFixture from sqlalchemy import create_engine +from sqlalchemy.dialects import sqlite from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.session import Session @@ -45,10 +46,121 @@ validate_rendered_expression, ) from superset.sql.parse import Table -from superset.superset_typing import QueryObjectDict +from superset.superset_typing import AdhocMetric, QueryObjectDict from superset.utils import json +def test_get_sqla_col_quotes_snowflake_case_sensitive_identifier( + mocker: MockerFixture, +) -> None: + """Snowflake physical columns retain their exact reflected case in generated SQL.""" + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + + database = Database(database_name="db", sqlalchemy_uri="sqlite://") + mocker.patch.object( + Database, + "get_db_engine_spec", + return_value=SnowflakeEngineSpec, + ) + table = SqlaTable( + table_name="bug_test", + database=database, + normalize_columns=False, + ) + tbl_column = TableColumn(column_name="id", type="INTEGER", table=table) + + rendered = str( + tbl_column.get_sqla_col().compile( + dialect=sqlite.dialect(), + compile_kwargs={"literal_binds": True}, + ) + ) + + assert rendered == '"id"' + + +@pytest.mark.parametrize("time_grain", [None, "P1D"]) +def test_get_timestamp_expression_quotes_snowflake_case_sensitive_identifier( + mocker: MockerFixture, + time_grain: str | None, +) -> None: + """Snowflake timestamp paths quote exact-case physical columns.""" + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + + database = Database(database_name="db", sqlalchemy_uri="sqlite://") + mocker.patch.object( + Database, + "get_db_engine_spec", + return_value=SnowflakeEngineSpec, + ) + table = SqlaTable( + table_name="bug_test", + database=database, + normalize_columns=False, + ) + tbl_column = TableColumn( + column_name="created_at", + type="TIMESTAMP", + table=table, + ) + + rendered = str( + tbl_column.get_timestamp_expression(time_grain=time_grain).compile( + dialect=sqlite.dialect(), + compile_kwargs={"literal_binds": True}, + ) + ) + + assert '"created_at"' in rendered + + +def test_adhoc_metric_to_sqla_quotes_snowflake_column_absent_from_columns_by_name( + mocker: MockerFixture, +) -> None: + """A SIMPLE adhoc metric quotes exact-case Snowflake columns even when the + metric's column is unknown to the dataset. + + ``adhoc_metric_to_sqla`` only routes through ``TableColumn.get_sqla_col`` when + the column is present in ``columns_by_name``; the fallback builds a bare + ``column()`` and must apply the same identifier preparation, otherwise + SQLAlchemy upper-cases the unquoted name and Snowflake fails to resolve it. + """ + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + + database = Database(database_name="db", sqlalchemy_uri="sqlite://") + mocker.patch.object( + Database, + "get_db_engine_spec", + return_value=SnowflakeEngineSpec, + ) + table = SqlaTable( + table_name="bug_test", + database=database, + normalize_columns=False, + ) + metric: AdhocMetric = { + "expressionType": "SIMPLE", + "aggregate": "SUM", + "column": {"column_name": "amount"}, + "label": "total", + } + + # Deliberately empty so the lookup misses and the fallback branch runs. + sqla_metric = table.adhoc_metric_to_sqla(metric, {}) + + rendered = str( + sqla_metric.compile( + dialect=sqlite.dialect(), + compile_kwargs={"literal_binds": True}, + ) + ) + + assert '"amount"' in rendered, ( + f"Expected the exact-case column to be quoted, got: {rendered}" + ) + assert "(amount)" not in rendered, f"Column was aggregated unquoted: {rendered}" + + def test_query_bubbles_errors(mocker: MockerFixture) -> None: """ Test that the `query` method bubbles exceptions correctly. diff --git a/tests/unit_tests/datasource/dao_tests.py b/tests/unit_tests/datasource/dao_tests.py index a55b8058b617..5e1061640513 100644 --- a/tests/unit_tests/datasource/dao_tests.py +++ b/tests/unit_tests/datasource/dao_tests.py @@ -16,6 +16,7 @@ # under the License. from collections.abc import Iterator +from typing import Any import pytest from sqlalchemy import literal, select @@ -147,6 +148,48 @@ def test_escape_ilike_fragment() -> None: assert _escape_ilike_fragment("foo%bar_baz\\") == "foo\\%bar\\_baz\\\\" +def test_paginate_combined_query_orders_equal_names_deterministically( + session: Session, +) -> None: + """Rows that tie on the sort column must not shuffle between pages.""" + from sqlalchemy import union_all + from sqlalchemy.sql.selectable import Select + + from superset.daos.datasource import DatasourceDAO + + def row(item_id: int, source_type: str, table_name: str) -> Select[Any]: + return select( + literal(item_id).label("item_id"), + literal(source_type).label("source_type"), + literal("2026-01-01").label("changed_on"), + literal(table_name).label("table_name"), + ) + + combined = union_all( + row(2, "semantic_layer", "orders"), + row(3, "database", "orders"), + row(1, "semantic_layer", "orders"), + row(1, "database", "orders"), + ).subquery() + + def page(index: int) -> list[tuple[int, str]]: + _, rows = DatasourceDAO.paginate_combined_query( + combined=combined, + order_column="table_name", + order_direction="asc", + page=index, + page_size=2, + ) + return [(r.item_id, r.source_type) for r in rows] + + assert page(0) + page(1) == [ + (1, "database"), + (3, "database"), + (1, "semantic_layer"), + (2, "semantic_layer"), + ] + + def test_paginate_combined_query_invalid_sort_column() -> None: from superset.daos.datasource import DatasourceDAO diff --git a/tests/unit_tests/db_engine_specs/test_base.py b/tests/unit_tests/db_engine_specs/test_base.py index ece1bb2c0203..13c6d8965b13 100644 --- a/tests/unit_tests/db_engine_specs/test_base.py +++ b/tests/unit_tests/db_engine_specs/test_base.py @@ -291,6 +291,13 @@ def test_get_default_catalog(mocker: MockerFixture) -> None: assert BaseEngineSpec.get_default_catalog(database) is None +def test_prepare_identifier_returns_name_unchanged() -> None: + name = "physical_column" + + assert BaseEngineSpec.prepare_identifier(name, normalize_columns=False) is name + assert BaseEngineSpec.prepare_identifier(name, normalize_columns=True) is name + + def test_quote_table() -> None: """ Test the `quote_table` function. @@ -1210,21 +1217,19 @@ def test_get_oauth2_fresh_token_success(mocker: MockerFixture) -> None: assert result == {"access_token": "new-access-token", "expires_in": 3600} -@pytest.mark.parametrize("status_code", [400, 401, 403]) @with_config({"DATABASE_OAUTH2_TIMEOUT": timedelta(seconds=30)}) -def test_get_oauth2_fresh_token_raises_on_auth_error( +def test_get_oauth2_fresh_token_raises_on_invalid_grant( mocker: MockerFixture, - status_code: int, ) -> None: """ - Test that get_oauth2_fresh_token raises OAuth2TokenRefreshError on 400/401/403. + Test that a definitive refresh-token rejection requests interactive OAuth2. """ from superset.db_engine_specs.base import BaseEngineSpec from superset.exceptions import OAuth2TokenRefreshError mock_post = mocker.patch("superset.db_engine_specs.base.requests.post") - mock_post.return_value.status_code = status_code - mock_post.return_value.text = '{"error": "provider-payload-sentinel"}' + mock_post.return_value.status_code = 400 + mock_post.return_value.json.return_value = {"error": "invalid_grant"} config: OAuth2ClientConfig = { "id": "client-id", @@ -1239,7 +1244,40 @@ def test_get_oauth2_fresh_token_raises_on_auth_error( with pytest.raises(OAuth2TokenRefreshError) as exc_info: BaseEngineSpec.get_oauth2_fresh_token(config, "refresh-token") - assert "provider-payload-sentinel" not in str(exc_info.value.to_dict()) + assert "invalid_grant" not in str(exc_info.value.to_dict()) + + +@pytest.mark.parametrize( + "status_code,error", [(400, "temporarily_unavailable"), (401, "invalid_client")] +) +@with_config({"DATABASE_OAUTH2_TIMEOUT": timedelta(seconds=30)}) +def test_get_oauth2_fresh_token_preserves_token_on_ambiguous_error( + mocker: MockerFixture, + status_code: int, + error: str, +) -> None: + """Non-invalid_grant responses remain ordinary provider failures.""" + from requests.exceptions import HTTPError + + from superset.db_engine_specs.base import BaseEngineSpec + + mock_post = mocker.patch("superset.db_engine_specs.base.requests.post") + mock_post.return_value.status_code = status_code + mock_post.return_value.json.return_value = {"error": error} + mock_post.return_value.raise_for_status.side_effect = HTTPError() + + config: OAuth2ClientConfig = { + "id": "client-id", + "secret": "client-secret", + "scope": "read write", + "redirect_uri": "http://localhost:8088/api/v1/database/oauth2/", + "authorization_request_uri": "https://oauth.example.com/authorize", + "token_request_uri": "https://oauth.example.com/token", + "request_content_type": "json", + } + + with pytest.raises(HTTPError): + BaseEngineSpec.get_oauth2_fresh_token(config, "refresh-token") @with_config({"DATABASE_OAUTH2_TIMEOUT": timedelta(seconds=30)}) diff --git a/tests/unit_tests/db_engine_specs/test_gsheets.py b/tests/unit_tests/db_engine_specs/test_gsheets.py index fc77cf57732f..4a156a0f5d62 100644 --- a/tests/unit_tests/db_engine_specs/test_gsheets.py +++ b/tests/unit_tests/db_engine_specs/test_gsheets.py @@ -110,6 +110,37 @@ def test_validate_parameters_no_catalog(mocker: MockerFixture) -> None: ] +def test_validate_parameters_malformed_credentials(mocker: MockerFixture) -> None: + from superset.db_engine_specs.gsheets import ( + GSheetsEngineSpec, + GSheetsPropertiesType, + ) + + g = mocker.patch("superset.db_engine_specs.gsheets.g") + g.user.email = "admin@example.org" + + properties: GSheetsPropertiesType = { + "parameters": { + "service_account_info": "{not valid json", + "catalog": {}, + }, + "catalog": {}, + } + errors = GSheetsEngineSpec.validate_parameters(properties) + assert errors == [ + SupersetError( + message=( + "The service account credentials are not valid JSON. " + "Please check that the field contains a valid service " + "account key." + ), + error_type=SupersetErrorType.INVALID_PAYLOAD_FORMAT_ERROR, + level=ErrorLevel.ERROR, + extra={"invalid": ["service_account_info"]}, + ), + ] + + def test_validate_parameters_simple_with_in_root_catalog(mocker: MockerFixture) -> None: from superset.db_engine_specs.gsheets import ( GSheetsEngineSpec, diff --git a/tests/unit_tests/db_engine_specs/test_snowflake.py b/tests/unit_tests/db_engine_specs/test_snowflake.py index d41c8cbc4b77..c7f14e9e3372 100644 --- a/tests/unit_tests/db_engine_specs/test_snowflake.py +++ b/tests/unit_tests/db_engine_specs/test_snowflake.py @@ -24,6 +24,7 @@ import pytest from pytest_mock import MockerFixture from sqlalchemy.engine.url import make_url, URL +from sqlalchemy.sql import quoted_name from superset.app import SupersetApp from superset.errors import ErrorLevel, SupersetError, SupersetErrorType @@ -33,6 +34,32 @@ from tests.unit_tests.fixtures.common import dttm # noqa: F401 +@pytest.mark.parametrize("name", ["lowercase", "UPPERCASE"]) +def test_prepare_identifier_quotes_exact_case_names(name: str) -> None: + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + + identifier = SnowflakeEngineSpec.prepare_identifier( + name, + normalize_columns=False, + ) + + assert isinstance(identifier, quoted_name) + assert str(identifier) == name + assert identifier.quote is True + + +def test_prepare_identifier_preserves_normalized_name() -> None: + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + + name = "lowercase" + identifier = SnowflakeEngineSpec.prepare_identifier( + name, + normalize_columns=True, + ) + + assert identifier is name + + @pytest.mark.parametrize( "target_type,expected_result", [ @@ -703,6 +730,18 @@ def test_custom_snowflake_auth_error_matches_raw_dbapi_exception() -> None: assert isinstance(raw_error, CustomSnowflakeAuthError) +def test_custom_snowflake_auth_error_matches_snowflake_error_code() -> None: + """Snowflake's documented OAuth access-token error code is authoritative.""" + from superset.db_engine_specs.snowflake import ( + CustomSnowflakeAuthError, + DatabaseError, + ) + + raw_error = DatabaseError("authentication failed") + raw_error.errno = 390303 + assert isinstance(raw_error, CustomSnowflakeAuthError) + + def test_custom_snowflake_auth_error_matches_sqlalchemy_wrapped_exception() -> None: """ Some call sites execute through SQLAlchemy's `Engine`, which wraps the diff --git a/tests/unit_tests/jinja_context_test.py b/tests/unit_tests/jinja_context_test.py index fe1b504c83af..441966235cb2 100644 --- a/tests/unit_tests/jinja_context_test.py +++ b/tests/unit_tests/jinja_context_test.py @@ -1441,6 +1441,31 @@ def test_metric_macro_no_dataset_id_no_context(mocker: MockerFixture) -> None: DatasetDAO.find_by_id.assert_not_called() +def test_metric_macro_no_dataset_id_non_json_body_with_json_content_type( + mocker: MockerFixture, +) -> None: + """ + Test the ``metric_macro`` when the request context's Content-Type claims + JSON but the body isn't parseable JSON -- the shape of the request an MCP + tool call runs in. Previously ``request.get_json()`` let a raw Werkzeug + ``BadRequest`` escape here instead of falling through to the + dataset-not-specified path. + """ + DatasetDAO = mocker.patch("superset.daos.dataset.DatasetDAO") # noqa: N806 + mock_g = mocker.patch("superset.jinja_context.g") + mock_g.form_data = {} + env = SandboxedEnvironment(undefined=DebugUndefined) + with current_app.test_request_context( + data="not-json-at-all", content_type="application/json" + ): + with pytest.raises(SupersetTemplateException) as excinfo: + metric_macro(env, {}, "macro_key") + assert str(excinfo.value) == ( + "Please specify the Dataset ID for the ``macro_key`` metric in the Jinja macro." # noqa: E501 + ) + DatasetDAO.find_by_id.assert_not_called() + + def test_metric_macro_no_dataset_id_with_context_missing_info( mocker: MockerFixture, ) -> None: diff --git a/tests/unit_tests/models/helpers_test.py b/tests/unit_tests/models/helpers_test.py index d1b000ae4521..e5a7149594d7 100644 --- a/tests/unit_tests/models/helpers_test.py +++ b/tests/unit_tests/models/helpers_test.py @@ -4699,6 +4699,83 @@ def test_simple_metric_quotes_column_requiring_quoting(database: Database) -> No ) +def test_explore_mixin_adhoc_metric_quotes_snowflake_case_sensitive_identifier( + database: Database, +) -> None: + """``ExploreMixin.adhoc_metric_to_sqla`` quotes exact-case Snowflake columns. + + Unlike the ``SqlaTable`` override, this implementation builds the aggregate + from a bare ``sa.column()`` unconditionally, so every SIMPLE adhoc metric on + the query-object path bypassed identifier preparation. + """ + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + from superset.models.helpers import ExploreMixin + + datasource = MagicMock() + datasource.database = database + datasource.db_engine_spec = SnowflakeEngineSpec + datasource.normalize_columns = False + datasource.sqla_aggregations = ExploreMixin.sqla_aggregations + for method in ("adhoc_metric_to_sqla", "make_sqla_column_compatible"): + setattr(datasource, method, getattr(ExploreMixin, method).__get__(datasource)) + + metric: AdhocMetric = { + "expressionType": "SIMPLE", + "aggregate": "SUM", + "column": {"column_name": "amount"}, + "label": "total", + } + + with database.get_sqla_engine() as engine: + dialect = engine.dialect + + rendered = str( + datasource.adhoc_metric_to_sqla(metric, {}).compile( + dialect=dialect, + compile_kwargs={"literal_binds": True}, + ) + ) + + assert '"amount"' in rendered, ( + f"Expected the exact-case column to be quoted, got: {rendered}" + ) + assert "(amount)" not in rendered, f"Column was aggregated unquoted: {rendered}" + + +def test_convert_tbl_column_quotes_snowflake_case_sensitive_identifier( + database: Database, + mocker: MockerFixture, +) -> None: + """The chart query-object path quotes exact-case Snowflake physical columns.""" + from superset.connectors.sqla.models import SqlaTable, TableColumn + from superset.db_engine_specs.snowflake import SnowflakeEngineSpec + from superset.models.core import Database + + mocker.patch.object( + Database, + "get_db_engine_spec", + return_value=SnowflakeEngineSpec, + ) + table = SqlaTable( + database=database, + table_name="bug_test", + normalize_columns=False, + ) + tbl_column = TableColumn(column_name="name", type="VARCHAR", table=table) + + with database.get_sqla_engine() as engine: + dialect = engine.dialect + + rendered = str( + table.convert_tbl_column_to_sqla_col(tbl_column).compile( + dialect=dialect, + compile_kwargs={"literal_binds": True}, + ) + ) + + assert rendered == '"name"' + + @pytest.mark.parametrize( "native_type", [ diff --git a/tests/unit_tests/models/slice_test.py b/tests/unit_tests/models/slice_test.py index 7481fa4f4ed9..a92e14e51bd6 100644 --- a/tests/unit_tests/models/slice_test.py +++ b/tests/unit_tests/models/slice_test.py @@ -125,6 +125,87 @@ def test_datasource_url_returns_none_when_no_datasource(self): result = slc.datasource_url() assert result is None + @staticmethod + def _semantic_view_slice() -> Slice: + """Build a chart on a semantic view, with a colliding table also attached. + + The table stands in for a regular dataset that happens to share the + numeric id; it must never leak into the semantic-view chart's display. + """ + slc = Slice() + slc.id = 1 + slc.datasource_type = "semantic_view" + slc.datasource_id = 2 + view = MagicMock() + view.name = "orders" + view.url = "/semantic_view/abc/" + view.explore_url = "/explore/?datasource_type=semantic_view&datasource_id=2" + view.link = "orders" + slc.semantic_view = view + table = MagicMock() + table.name = "public.colliding_table" + table.explore_url = "/explore/?datasource_type=table&datasource_id=2" + slc.table = table + return slc + + def test_datasource_url_uses_semantic_view_explore_url(self) -> None: + """A semantic-view chart links to the view's Explore page, not a table's.""" + slc = self._semantic_view_slice() + + assert ( + slc.datasource_url() + == "/explore/?datasource_type=semantic_view&datasource_id=2" + ) + + def test_datasource_name_text_uses_semantic_view_name(self) -> None: + """A semantic-view chart is named after the view (no schema prefix).""" + slc = self._semantic_view_slice() + + assert slc.datasource_name_text() == "orders" + + def test_display_datasource_never_falls_back_across_types(self) -> None: + """A semantic-view chart with no view resolves to None, not to a table.""" + slc = self._semantic_view_slice() + slc.semantic_view = None + + assert slc._display_datasource() is None + assert slc.datasource_url() is None + assert slc.datasource_name_text() is None + + def test_table_chart_display_is_unchanged_by_semantic_view_relationship( + self, + ) -> None: + """A table chart ignores ``semantic_view`` even if it is populated.""" + slc = self._semantic_view_slice() + slc.datasource_type = "table" + + assert slc.datasource_url() == "/explore/?datasource_type=table&datasource_id=2" + assert slc.datasource_name_text() == "public.colliding_table" + + def test_datasource_edit_url_and_link_use_semantic_view(self) -> None: + """Edit URL and legacy link come from the view for a semantic-view chart.""" + slc = self._semantic_view_slice() + + assert slc.datasource_edit_url == "/semantic_view/abc/" + assert slc.datasource_link() == "orders" + + def test_datasource_link_is_none_when_unresolved(self) -> None: + """A chart whose datasource cannot be resolved has no link, no error.""" + slc = self._semantic_view_slice() + slc.semantic_view = None + + assert slc.datasource_link() is None + assert slc.datasource_edit_url is None + + def test_icons_names_the_semantic_view(self) -> None: + """icons uses the semantic view's name and edit URL for its tooltip.""" + slc = self._semantic_view_slice() + + html = slc.icons + + assert 'title="orders"' in html + assert 'href="/semantic_view/abc/"' in html + def test_icons_escapes_datasource_html(self): """icons must HTML-escape the datasource name and edit URL.""" slc = Slice() @@ -137,8 +218,7 @@ def test_icons_escapes_datasource_html(self): ), patch.object( Slice, - "datasource", - new_callable=PropertyMock, + "datasource_name_text", return_value="", ), ): diff --git a/tests/unit_tests/reports/api_test.py b/tests/unit_tests/reports/api_test.py index 280cb108ba8c..95dcb26645b1 100644 --- a/tests/unit_tests/reports/api_test.py +++ b/tests/unit_tests/reports/api_test.py @@ -19,7 +19,10 @@ import rison -from superset.exceptions import SupersetException +from superset.utils.slack import ( + SlackChannelListingClientError, + SlackChannelListingError, +) from tests.unit_tests.conftest import with_feature_flags @@ -80,14 +83,43 @@ def test_slack_channels_page_without_page_size_returns_all( @with_feature_flags(ALERT_REPORTS=True) +@patch("superset.reports.api.logger") @patch("superset.reports.api.get_channels_with_search") -def test_slack_channels_handles_superset_exception( +def test_slack_channels_client_error_logs_warning( mock_search: Any, + logger_mock: Any, client: Any, full_api_access: None, ) -> None: - mock_search.side_effect = SupersetException("Slack API error") + # A permanent token/client-setup failure (e.g. a revoked bot token) is + # expected, already-handled noise, so it must be logged at WARNING, not + # ERROR, to avoid polluting Sentry with an actionable-looking signal. + mock_search.side_effect = SlackChannelListingClientError("Slack API error") params = rison.dumps({}) rv = client.get(f"/api/v1/report/slack_channels/?q={params}") assert rv.status_code == 422 assert "Slack API error" in rv.json["message"] + logger_mock.error.assert_not_called() + logger_mock.warning.assert_called_once() + assert "Slack API error" in logger_mock.warning.call_args.args[1] + + +@with_feature_flags(ALERT_REPORTS=True) +@patch("superset.reports.api.logger") +@patch("superset.reports.api.get_channels_with_search") +def test_slack_channels_transient_error_logs_error( + mock_search: Any, + logger_mock: Any, + client: Any, + full_api_access: None, +) -> None: + # A transient listing failure (rate limits, transport errors) means Slack is + # unavailable, so it must stay ERROR to preserve an actionable signal. + mock_search.side_effect = SlackChannelListingError("Slack API error") + params = rison.dumps({}) + rv = client.get(f"/api/v1/report/slack_channels/?q={params}") + assert rv.status_code == 422 + assert "Slack API error" in rv.json["message"] + logger_mock.warning.assert_not_called() + logger_mock.error.assert_called_once() + assert "Slack API error" in logger_mock.error.call_args.args[1] diff --git a/tests/unit_tests/security/manager_test.py b/tests/unit_tests/security/manager_test.py index 27e730159863..c78ec9dd6741 100644 --- a/tests/unit_tests/security/manager_test.py +++ b/tests/unit_tests/security/manager_test.py @@ -3021,6 +3021,112 @@ def test_get_catalogs_accessible_by_user_schema_access( assert sm.get_catalogs_accessible_by_user(database, catalogs) == {"catalog2"} +def test_get_schemas_accessible_by_user_cached_list( + mocker: MockerFixture, + app_context: None, +) -> None: + """ + Test that `get_schemas_accessible_by_user` handles candidate names that a cache + serializer deserialized as a list instead of a set. Before normalization this + raised `TypeError: unsupported operand type(s) for &: 'list' and 'set'` for users + with only schema-level access. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "can_access_database", return_value=False) + mocker.patch.object( + sm, + "user_view_menu_names", + side_effect=[ + {"[db1].[schema2]"}, # schema_access + set(), # datasource_access + ], + ) + + database = mocker.MagicMock() + database.database_name = "db1" + database.get_default_catalog.return_value = None + database.get_default_schema.return_value = None + + schemas = ["schema1", "schema2"] + + assert sm.get_schemas_accessible_by_user(database, None, schemas) == {"schema2"} + + +def test_get_schemas_accessible_by_user_hierarchical_cached_list( + mocker: MockerFixture, + app_context: None, +) -> None: + """ + Test that the hierarchical early return normalizes a list-typed candidate + collection to a set, so callers always receive a `set` regardless of the cache + serializer. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "can_access_database", return_value=True) + + database = mocker.MagicMock() + database.database_name = "db1" + database.get_default_catalog.return_value = None + database.get_default_schema.return_value = None + + schemas = ["schema1", "schema2"] + + result = sm.get_schemas_accessible_by_user(database, None, schemas) + assert result == {"schema1", "schema2"} + assert isinstance(result, set) + + +def test_get_catalogs_accessible_by_user_cached_list( + mocker: MockerFixture, + app_context: None, +) -> None: + """ + Test that `get_catalogs_accessible_by_user` handles candidate names that a cache + serializer deserialized as a list instead of a set. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "can_access_database", return_value=False) + mocker.patch.object( + sm, + "user_view_menu_names", + side_effect=[ + set(), # catalog_access + {"[db1].[catalog2].[schema1]"}, # schema_access + set(), # datasource_access + ], + ) + + database = mocker.MagicMock() + database.database_name = "db1" + database.get_default_catalog.return_value = "catalog2" + + catalogs = ["catalog1", "catalog2"] + + assert sm.get_catalogs_accessible_by_user(database, catalogs) == {"catalog2"} + + +def test_get_catalogs_accessible_by_user_hierarchical_cached_list( + mocker: MockerFixture, + app_context: None, +) -> None: + """ + Test that the hierarchical early return for catalogs normalizes a list-typed + candidate collection to a set. + """ + sm = SupersetSecurityManager(appbuilder) + mocker.patch.object(sm, "can_access_database", return_value=True) + + database = mocker.MagicMock() + database.database_name = "db1" + database.get_default_catalog.return_value = "catalog2" + + catalogs = ["catalog1", "catalog2"] + + result = sm.get_catalogs_accessible_by_user(database, catalogs) + assert result == {"catalog1", "catalog2"} + assert isinstance(result, set) + + def test_get_rls_filters_uses_table_id_directly( mocker: MockerFixture, app_context: None, diff --git a/tests/unit_tests/utils/oauth2_tests.py b/tests/unit_tests/utils/oauth2_tests.py index 4584a71fc747..6be95461efbe 100644 --- a/tests/unit_tests/utils/oauth2_tests.py +++ b/tests/unit_tests/utils/oauth2_tests.py @@ -27,6 +27,9 @@ import pytest from freezegun import freeze_time from pytest_mock import MockerFixture +from sqlalchemy import Column, create_engine, DateTime, Integer, String +from sqlalchemy.orm import declarative_base, sessionmaker +from sqlalchemy.pool import StaticPool from superset.db_engine_specs.base import BaseEngineSpec from superset.exceptions import ( @@ -39,6 +42,7 @@ check_for_oauth2, decode_oauth2_state, encode_oauth2_state, + execute_with_oauth2_retry, generate_code_challenge, generate_code_verifier, get_oauth2_access_token, @@ -119,6 +123,259 @@ def test_get_oauth2_access_token_base_no_refresh(mocker: MockerFixture) -> None: db.session.delete.assert_called_with(token) +def test_refresh_oauth2_token_force_refreshes_valid_token( + mocker: MockerFixture, +) -> None: + """A forced refresh must not reuse an unexpired access token.""" + db = mocker.patch("superset.utils.oauth2.db") + mocker.patch("superset.utils.oauth2.Session", return_value=db.session) + mocker.patch("superset.utils.oauth2.DistributedLock") + db_engine_spec = mocker.MagicMock() + db_engine_spec.get_oauth2_fresh_token.return_value = { + "access_token": "new-token", + "expires_in": 3600, + } + token = mocker.MagicMock() + token.access_token = "stale-token" # noqa: S105 + token.access_token_expiration = datetime(2024, 1, 2) + token.refresh_token = "refresh-token" # noqa: S105 + db.session.query().populate_existing().filter_by().one_or_none.return_value = token + + with freeze_time("2024-01-01"): + result = refresh_oauth2_token( + DUMMY_OAUTH2_CONFIG, 1, 1, db_engine_spec, force=True + ) + + assert result == "new-token" + db_engine_spec.get_oauth2_fresh_token.assert_called_once_with( + DUMMY_OAUTH2_CONFIG, "refresh-token" + ) + db.session.commit.assert_called_once_with() + + +def test_force_refresh_reuses_concurrently_refreshed_token( + mocker: MockerFixture, +) -> None: + """A lock waiter must not exchange a rotated refresh token again.""" + db = mocker.patch("superset.utils.oauth2.db") + mocker.patch("superset.utils.oauth2.Session", return_value=db.session) + mocker.patch("superset.utils.oauth2.DistributedLock") + db_engine_spec = mocker.MagicMock() + token = mocker.MagicMock(access_token="winning-token") # noqa: S106 + db.session.query().populate_existing().filter_by().one_or_none.return_value = token + + result = refresh_oauth2_token( + DUMMY_OAUTH2_CONFIG, + 1, + 1, + db_engine_spec, + force=True, + rejected_access_token="rejected-token", # noqa: S106 + ) + + assert result == "winning-token" + db_engine_spec.get_oauth2_fresh_token.assert_not_called() + db.session.delete.assert_not_called() + + +def test_force_refresh_commits_deletion_without_refresh_token( + mocker: MockerFixture, +) -> None: + """The isolated forced-refresh session durably removes unusable rows.""" + db = mocker.patch("superset.utils.oauth2.db") + mocker.patch("superset.utils.oauth2.Session", return_value=db.session) + mocker.patch("superset.utils.oauth2.DistributedLock") + token = mocker.MagicMock( + access_token="rejected-token", # noqa: S106 + refresh_token=None, + ) + db.session.query().populate_existing().filter_by().one_or_none.return_value = token + + result = refresh_oauth2_token( + DUMMY_OAUTH2_CONFIG, + 1, + 1, + mocker.MagicMock(), + force=True, + rejected_access_token="rejected-token", # noqa: S106 + ) + + assert result is None + db.session.delete.assert_called_once_with(token) + db.session.commit.assert_called_once_with() + + +def test_execute_with_oauth2_retry_forces_refresh_once( + mocker: MockerFixture, +) -> None: + """A tightly classified auth error triggers one refresh and one retry.""" + auth_error = RuntimeError("stale OAuth token") + operation = mocker.Mock(side_effect=[auth_error, "result"]) + database = mocker.MagicMock() + database.id = 1 + database.is_oauth2_enabled.return_value = True + database.db_engine_spec.engine = "snowflake" + database.db_engine_spec.needs_oauth2.return_value = True + database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG + mocker.patch("superset.utils.oauth2.g").user.id = 2 + db = mocker.patch("superset.utils.oauth2.db") + token = mocker.MagicMock(access_token="stale-token") # noqa: S106 + db.session.query().filter_by().one_or_none.return_value = token + refresh = mocker.patch( + "superset.utils.oauth2.refresh_oauth2_token", return_value="new-token" + ) + + assert execute_with_oauth2_retry(database, operation) == "result" + + assert operation.call_count == 2 + refresh.assert_called_once_with( + DUMMY_OAUTH2_CONFIG, + 1, + 2, + database.db_engine_spec, + force=True, + rejected_access_token="stale-token", # noqa: S106 + ) + db.session.expire.assert_called_once_with(token) + + +def test_execute_with_oauth2_retry_expires_token_from_ambient_session( + mocker: MockerFixture, +) -> None: + """The retry observes a forced refresh committed by an isolated session.""" + base = declarative_base() + + class OAuthToken(base): # type: ignore[valid-type,misc] + __tablename__ = "oauth_token" + + id = Column(Integer, primary_key=True) + user_id = Column(Integer, nullable=False) + database_id = Column(Integer, nullable=False) + access_token = Column(String, nullable=True) + access_token_expiration = Column(DateTime, nullable=True) + refresh_token = Column(String, nullable=True) + + engine = create_engine( + "sqlite://", + connect_args={"check_same_thread": False}, + poolclass=StaticPool, + ) + base.metadata.create_all(engine) + ambient_session = sessionmaker(bind=engine)() + ambient_session.add( + OAuthToken( + user_id=2, + database_id=1, + access_token="stale-token", # noqa: S106 + access_token_expiration=datetime(2024, 1, 2), + refresh_token="refresh-token", # noqa: S106 + ) + ) + ambient_session.commit() + + db = mocker.patch("superset.utils.oauth2.db") + db.session = ambient_session + mocker.patch("superset.models.core.DatabaseUserOAuth2Tokens", OAuthToken) + mocker.patch("superset.utils.oauth2.DistributedLock") + mocker.patch("superset.utils.oauth2.g").user.id = 2 + + auth_error = RuntimeError("stale OAuth token") + observed_tokens: list[str | None] = [] + + def operation() -> str: + if not observed_tokens: + observed_tokens.append(None) + raise auth_error + observed_tokens.append( + ambient_session.query(OAuthToken) + .filter_by(user_id=2, database_id=1) + .one() + .access_token + ) + return "result" + + database = mocker.MagicMock(id=1) + database.is_oauth2_enabled.return_value = True + database.db_engine_spec.engine = "snowflake" + database.db_engine_spec.needs_oauth2.return_value = True + database.db_engine_spec.oauth2_exception = OAuth2TokenRefreshError + database.db_engine_spec.get_oauth2_fresh_token.return_value = { + "access_token": "new-token", + "expires_in": 3600, + "refresh_token": "rotated-refresh-token", + } + database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG + + assert execute_with_oauth2_retry(database, operation) == "result" + assert observed_tokens == [None, "new-token"] + + +def test_execute_with_oauth2_retry_does_not_retry_unrelated_error( + mocker: MockerFixture, +) -> None: + """Network and other unclassified failures must not discard valid tokens.""" + network_error = RuntimeError("connection timed out") + operation = mocker.Mock(side_effect=network_error) + database = mocker.MagicMock() + database.is_oauth2_enabled.return_value = True + database.db_engine_spec.needs_oauth2.return_value = False + db = mocker.patch("superset.utils.oauth2.db") + db.session.query().filter_by().one_or_none.return_value = None + refresh = mocker.patch("superset.utils.oauth2.refresh_oauth2_token") + + with pytest.raises(RuntimeError, match="connection timed out"): + execute_with_oauth2_retry(database, operation) + + operation.assert_called_once_with() + refresh.assert_not_called() + + +def test_execute_with_oauth2_retry_propagates_second_auth_error( + mocker: MockerFixture, +) -> None: + """A second authentication failure is surfaced without another refresh.""" + auth_error = RuntimeError("stale OAuth token") + operation = mocker.Mock(side_effect=auth_error) + database = mocker.MagicMock(id=1) + database.is_oauth2_enabled.return_value = True + database.db_engine_spec.needs_oauth2.return_value = True + database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG + mocker.patch("superset.utils.oauth2.g").user.id = 2 + db = mocker.patch("superset.utils.oauth2.db") + db.session.query().filter_by().one_or_none.return_value = None + refresh = mocker.patch( + "superset.utils.oauth2.refresh_oauth2_token", return_value="new-token" + ) + + with pytest.raises(RuntimeError, match="stale OAuth token"): + execute_with_oauth2_retry(database, operation) + + assert operation.call_count == 2 + refresh.assert_called_once() + + +def test_execute_with_oauth2_retry_does_not_replay_after_progress( + mocker: MockerFixture, +) -> None: + """Completed statements prevent replay of a multi-statement query.""" + auth_error = RuntimeError("stale OAuth token") + operation = mocker.Mock(side_effect=auth_error) + database = mocker.MagicMock(id=1) + database.is_oauth2_enabled.return_value = True + database.db_engine_spec.needs_oauth2.return_value = True + mocker.patch("superset.utils.oauth2.g").user.id = 2 + db = mocker.patch("superset.utils.oauth2.db") + db.session.query().filter_by().one_or_none.return_value = None + refresh = mocker.patch("superset.utils.oauth2.refresh_oauth2_token") + + with pytest.raises(RuntimeError, match="stale OAuth token"): + execute_with_oauth2_retry(database, operation, can_retry=lambda: False) + + operation.assert_called_once_with() + refresh.assert_not_called() + database.start_oauth2_dance.assert_called_once_with() + + def test_refresh_oauth2_token_deletes_token_on_oauth2_exception( mocker: MockerFixture, caplog: pytest.LogCaptureFixture, diff --git a/tests/unit_tests/utils/slack_test.py b/tests/unit_tests/utils/slack_test.py index d9ed9227a40b..cc90266995f8 100644 --- a/tests/unit_tests/utils/slack_test.py +++ b/tests/unit_tests/utils/slack_test.py @@ -237,6 +237,62 @@ def test_handle_slack_client_error_listing_channels(self, mocker): The server responded with: missing scope: channels:read""" ) + @pytest.mark.parametrize( + "error_code", + [ + "not_authed", + "invalid_auth", + "account_inactive", + "token_revoked", + "token_expired", + ], + ) + def test_logs_slack_api_error_at_warning_not_error(self, error_code: str, mocker): + """An expired/revoked/inactive bot token is an expected multi-tenant + config state that is already handled end-to-end (re-raised as a + ``SupersetException`` and turned into a 422), so it should be logged + at WARNING, not ERROR, to avoid polluting Sentry.""" + from superset.exceptions import SupersetException + + mock_client = mocker.Mock() + mock_client.conversations_list.side_effect = SlackApiError( + message="foo", response={"ok": False, "error": error_code} + ) + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + logger_mock = mocker.patch("superset.utils.slack.logger") + + with pytest.raises(SupersetException): + get_channels_with_search() + + logger_mock.error.assert_not_called() + logger_mock.warning.assert_called_once() + assert "Failed to fetch Slack channels" in logger_mock.warning.call_args.args[0] + assert not logger_mock.warning.call_args.kwargs.get("exc_info") + + @pytest.mark.parametrize("error_code", ["ratelimited", "internal_error", ""]) + def test_logs_non_auth_slack_api_error_at_error_with_traceback( + self, error_code: str, mocker + ): + """Rate limits and Slack server/API errors are actionable outages, not + the expected auth-noise condition — they must keep ERROR-level logging + with a traceback so they still generate a Sentry event.""" + from superset.exceptions import SupersetException + + mock_client = mocker.Mock() + mock_client.conversations_list.side_effect = SlackApiError( + message="foo", response={"ok": False, "error": error_code} + ) + mocker.patch("superset.utils.slack.get_slack_client", return_value=mock_client) + logger_mock = mocker.patch("superset.utils.slack.logger") + + with pytest.raises(SupersetException): + get_channels_with_search() + + logger_mock.warning.assert_not_called() + logger_mock.error.assert_called_once() + assert "Failed to fetch Slack channels" in logger_mock.error.call_args.args[0] + assert logger_mock.error.call_args.kwargs.get("exc_info") is True + @pytest.mark.parametrize( ("error_code", "expected_exception"), [