Merge branch 'master' into enxdev/fix/sql-lab-save-dataset

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

.codecov.yml

@@ -3,9 +3,7 @@ codecov:
         after_n_builds: 4
 ignore:
   - "superset/migrations/versions/*.py"
-  - "superset-frontend/packages/superset-ui-demo/**/*"
   - "**/*.stories.tsx"
-  - "**/*.stories.jsx"
 coverage:
   status:
     project:

.github/workflows/mirror-service-images.yml

@@ -93,7 +93,7 @@ jobs:
           password: ${{ secrets.GITHUB_TOKEN }}
 
       - name: Set up Docker Buildx
-        uses: docker/setup-buildx-action@bb05f3f5519dd87d3ba754cc423b652a5edd6d2c # v4.2.0
+        uses: docker/setup-buildx-action@37fe631027851001ddb9b187196cc803df7f5f0e # v4.3.0
 
       - name: Copy image to GHCR
         env:

docs/package.json

@@ -66,7 +66,7 @@
     "caniuse-lite": "^1.0.30001809",
     "docusaurus-plugin-openapi-docs": "^5.2.0",
     "docusaurus-theme-openapi-docs": "^5.2.0",
-    "js-yaml": "^5.2.3",
+    "js-yaml": "^5.3.0",
     "json-bigint": "^1.0.0",
     "prism-react-renderer": "^2.4.1",
     "react": "^18.3.1",

docs/src/pages/community.tsx

@@ -67,7 +67,7 @@ const communityLinks = [
       'Join our monthly virtual meetups and register for any upcoming events on Meetup',
   },
   {
-    url: 'https://github.com/apache/superset/blob/master/RESOURCES/INTHEWILD.md',
+    url: 'https://superset.apache.org/inTheWild/',
     title: 'Organizations',
     description:
       'A list of some of the organizations using Superset in production.',

docs/yarn.lock

@@ -10291,10 +10291,10 @@ js-yaml@4.1.0, js-yaml@=4.3.1, js-yaml@^4.1.0, js-yaml@^4.1.1, js-yaml@^4.2.0, j
   dependencies:
     argparse "^2.0.1"
 
-js-yaml@^5.2.3:
-  version "5.2.3"
-  resolved "https://registry.yarnpkg.com/js-yaml/-/js-yaml-5.2.3.tgz#0942ae8f507e22eb0e54624871789cd477106e54"
-  integrity sha512-n+mUVyUX5bVv7G/G2zyIHOhdxfuU1dY2NOFzTQUWiMUbFss8b57NFlgCCaggU78wSw5KVS9cllzeLyzyR+n5nw==
+js-yaml@^5.3.0:
+  version "5.3.0"
+  resolved "https://registry.yarnpkg.com/js-yaml/-/js-yaml-5.3.0.tgz#526430a6da31065127528ae695ce168cfc5f91f0"
+  integrity sha512-muutsYr+e2+d3rTgUGslq5rxbBlUy3cJ61IsHag2QNDQV+7zXWjkUpmALIajhrlLlrgRUiymj6U3zUr/TMK84Q==
   dependencies:
     argparse "^2.0.1"
 

superset-frontend/package-lock.json

@@ -86,7 +86,7 @@
         "classnames": "^2.2.5",
         "content-disposition": "^2.0.1",
         "d3-scale": "^4.0.2",
-        "dayjs": "^1.11.21",
+        "dayjs": "^1.11.22",
         "dom-to-image-more": "^3.10.2",
         "dom-to-pdf": "^0.3.2",
         "echarts": "^6.1.0",
@@ -100,7 +100,7 @@
         "geostyler-style": "11.0.2",
         "geostyler-wfs-parser": "^3.0.1",
         "google-auth-library": "^11.0.2",
-        "immer": "^11.1.16",
+        "immer": "^11.1.17",
         "interweave": "^13.1.1",
         "jquery": "^4.0.0",
         "js-levenshtein": "^1.1.6",
@@ -220,7 +220,7 @@
         "babel-plugin-jsx-remove-data-test-id": "^3.0.0",
         "baseline-browser-mapping": "^2.11.14",
         "cheerio": "1.2.0",
-        "concurrently": "^10.0.4",
+        "concurrently": "^10.0.5",
         "copy-webpack-plugin": "^14.0.0",
         "cross-env": "^10.1.0",
         "css-loader": "^7.1.4",
@@ -17190,9 +17190,9 @@
       "license": "MIT"
     },
     "node_modules/concurrently": {
-      "version": "10.0.4",
-      "resolved": "https://registry.npmjs.org/concurrently/-/concurrently-10.0.4.tgz",
-      "integrity": "sha512-trZql+7l/0+WRAsAnEdctr4+iiOS6ZrViI6H8QWcCF9MFS/LT0dKpe8vluB1to6it+OxSI4VospFTIFMW8DJRw==",
+      "version": "10.0.5",
+      "resolved": "https://registry.npmjs.org/concurrently/-/concurrently-10.0.5.tgz",
+      "integrity": "sha512-JaP/CoftUrCcAFW/g//RbgEGwlelnEae6cfBLgH6ZdO6s8jPkn6p9SB9u6pdVxYXoiSnFqseOlHfrEfF82TVOg==",
       "dev": true,
       "license": "MIT",
       "dependencies": {
@@ -18541,9 +18541,9 @@
       }
     },
     "node_modules/dayjs": {
-      "version": "1.11.21",
-      "resolved": "https://registry.npmjs.org/dayjs/-/dayjs-1.11.21.tgz",
-      "integrity": "sha512-98IT+HOahAisibz/yjKbzuOBwYcjJ7BCLPzARyHiyEBmRz4fatF+KPJszEHXsGYjUG234aH/cOjW1wwTbKUZlA==",
+      "version": "1.11.22",
+      "resolved": "https://registry.npmjs.org/dayjs/-/dayjs-1.11.22.tgz",
+      "integrity": "sha512-1YRnxzt/AabP3GHxnaB9/b+ZScCKu5TeF+co+BWG+lnWVIwEcTFc1FVE0WLNmNO3sA6GGXL40i5qkHfbLzpwrg==",
       "license": "MIT"
     },
     "node_modules/debounce": {
@@ -23907,9 +23907,9 @@
       "license": "MIT"
     },
     "node_modules/immer": {
-      "version": "11.1.16",
-      "resolved": "https://registry.npmjs.org/immer/-/immer-11.1.16.tgz",
-      "integrity": "sha512-Xs7H9rBc+kti1J6RueUvbEBkmOz7jqj11XYgf+YMXAYzu8EeE7hwZ9poLXdVfVnGmJu7QAf41T7H2KuF6QoK6Q==",
+      "version": "11.1.17",
+      "resolved": "https://registry.npmjs.org/immer/-/immer-11.1.17.tgz",
+      "integrity": "sha512-8Vu44Y0MuMBlTQz/jQ8HEMYNq/bBqk87MnBwYR5mC8AthfhEXidZ5aT/oA/CUqboa8THKltnD9L3xyqhU/Sy1Q==",
       "license": "MIT",
       "funding": {
         "type": "opencollective",
@@ -34873,12 +34873,18 @@
       }
     },
     "node_modules/react-error-boundary": {
-      "version": "6.1.2",
-      "resolved": "https://registry.npmjs.org/react-error-boundary/-/react-error-boundary-6.1.2.tgz",
-      "integrity": "sha512-3DpCr5HVdZ0caUjYE/kIHBEJN0mNP3ZCgf16c48uJ5TbWjorKVp+YG8W3XqlJ7vJAVNw6wNIImyPXmFydwmyng==",
+      "version": "6.1.3",
+      "resolved": "https://registry.npmjs.org/react-error-boundary/-/react-error-boundary-6.1.3.tgz",
+      "integrity": "sha512-GnSKpCohFi2nQmJCWwP8O8wub7zexlePvpsejvQr35vS5RTouS1+utTNOmyc540yw5vyOXnSL1rBWsCQDmkyUA==",
       "license": "MIT",
       "peerDependencies": {
+        "@types/react": "^18.0.0 || ^19.0.0",
         "react": "^18.0.0 || ^19.0.0"
+      },
+      "peerDependenciesMeta": {
+        "@types/react": {
+          "optional": true
+        }
       }
     },
     "node_modules/react-google-recaptcha": {
@@ -42962,7 +42968,7 @@
         "d3-scale": "^4.0.2",
         "d3-time": "^3.1.0",
         "d3-time-format": "^4.1.0",
-        "dayjs": "^1.11.21",
+        "dayjs": "^1.11.22",
         "dompurify": "^3.4.13",
         "fetch-retry": "^6.0.0",
         "handlebars": "^4.7.9",
@@ -42974,7 +42980,7 @@
         "re-resizable": "^6.11.2",
         "react-ace": "^14.0.1",
         "react-draggable": "^4.7.1",
-        "react-error-boundary": "^6.1.2",
+        "react-error-boundary": "^6.1.3",
         "react-js-cron": "^6.0.2",
         "react-markdown": "^10.1.0",
         "react-resize-detector": "^7.1.2",
@@ -43073,15 +43079,6 @@
         "node": ">=12"
       }
     },
-    "packages/superset-ui-core/node_modules/dompurify": {
-      "version": "3.4.13",
-      "resolved": "https://registry.npmjs.org/dompurify/-/dompurify-3.4.13.tgz",
-      "integrity": "sha512-2vmYIoqjze2d+kakP8S/nS5shfsl587kzwEjcGlTdiksUVgFHnFCsLYDVj/JNqJVOQZGSYBTmuycv0PodwmnMQ==",
-      "license": "(MPL-2.0 OR Apache-2.0)",
-      "optionalDependencies": {
-        "@types/trusted-types": "^2.0.7"
-      }
-    },
     "packages/superset-ui-core/node_modules/react-ace": {
       "version": "14.0.1",
       "resolved": "https://registry.npmjs.org/react-ace/-/react-ace-14.0.1.tgz",

superset-frontend/package.json

@@ -163,7 +163,7 @@
     "classnames": "^2.2.5",
     "content-disposition": "^2.0.1",
     "d3-scale": "^4.0.2",
-    "dayjs": "^1.11.21",
+    "dayjs": "^1.11.22",
     "dom-to-image-more": "^3.10.2",
     "dom-to-pdf": "^0.3.2",
     "echarts": "^6.1.0",
@@ -177,7 +177,7 @@
     "geostyler-style": "11.0.2",
     "geostyler-wfs-parser": "^3.0.1",
     "google-auth-library": "^11.0.2",
-    "immer": "^11.1.16",
+    "immer": "^11.1.17",
     "interweave": "^13.1.1",
     "jquery": "^4.0.0",
     "js-levenshtein": "^1.1.6",
@@ -297,7 +297,7 @@
     "babel-plugin-jsx-remove-data-test-id": "^3.0.0",
     "baseline-browser-mapping": "^2.11.14",
     "cheerio": "1.2.0",
-    "concurrently": "^10.0.4",
+    "concurrently": "^10.0.5",
     "copy-webpack-plugin": "^14.0.0",
     "cross-env": "^10.1.0",
     "css-loader": "^7.1.4",

superset-frontend/packages/superset-ui-core/package.json

@@ -67,7 +67,7 @@
     "d3-scale": "^4.0.2",
     "d3-time": "^3.1.0",
     "d3-time-format": "^4.1.0",
-    "dayjs": "^1.11.21",
+    "dayjs": "^1.11.22",
     "dompurify": "^3.4.13",
     "fetch-retry": "^6.0.0",
     "handlebars": "^4.7.9",
@@ -79,7 +79,7 @@
     "re-resizable": "^6.11.2",
     "react-ace": "^14.0.1",
     "react-draggable": "^4.7.1",
-    "react-error-boundary": "^6.1.2",
+    "react-error-boundary": "^6.1.3",
     "react-js-cron": "^6.0.2",
     "react-markdown": "^10.1.0",
     "react-resize-detector": "^7.1.2",

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

@@ -43,6 +43,23 @@ describe('SaveDatasetActionButton', () => {
     expect(saveDatasetBtn).toBeVisible();
   });
 
+  test('disables only the dataset button when canSaveDataset is false', () => {
+    const onSaveAsExplore = jest.fn();
+    render(
+      <SaveDatasetActionButton
+        setShowSave={() => true}
+        onSaveAsExplore={onSaveAsExplore}
+        canSaveDataset={false}
+      />,
+    );
+
+    // Saving the query needs no results.
+    expect(screen.getByRole('button', { name: 'Save' })).toBeEnabled();
+    expect(
+      screen.getByRole('button', { name: /save dataset/i }),
+    ).toBeDisabled();
+  });
+
   test('disables the save dataset button when the query did not run successfully', async () => {
     render(
       <SaveDatasetActionButton

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

@@ -19,12 +19,14 @@
 import { act, type ComponentProps } from 'react';
 import {
   cleanup,
+  createStore,
   fireEvent,
   render,
   screen,
   userEvent,
   waitFor,
 } from 'spec/helpers/testing-library';
+import reducerIndex from 'spec/helpers/reducerIndex';
 import fetchMock from 'fetch-mock';
 import { SaveDatasetModal } from 'src/SqlLab/components/SaveDatasetModal';
 import { createDatasource } from 'src/SqlLab/actions/sqlLab';
@@ -63,6 +65,12 @@ beforeEach(() => {
   cleanup();
 });
 
+afterEach(() => {
+  // In-body restores are skipped when an assertion throws, leaking a
+  // configured spy into later tests.
+  jest.restoreAllMocks();
+});
+
 // Mock createDatasource to return a thunk that resolves with the dataset's
 // new id. The test's mock store includes redux-thunk middleware (from RTK's
 // getDefaultMiddleware), so dispatch(createDatasource(...)) properly unwraps
@@ -518,6 +526,39 @@ describe('SaveDatasetModal', () => {
     });
   });
 
+  test('surfaces the error and keeps the modal open when saving fails', async () => {
+    // The chart-payload step's toast was built but never dispatched, so a
+    // failure there was silent.
+    const postFormData = jest.spyOn(
+      require('src/explore/exploreUtils/formData'),
+      'postFormData',
+    );
+    postFormData.mockRejectedValue(new Error('Boom'));
+    const onHide = jest.fn();
+    const store = createStore({ user }, reducerIndex);
+
+    render(<SaveDatasetModal {...mockedProps} onHide={onHide} />, { store });
+
+    fireEvent.change(screen.getByDisplayValue(/unimportant/i), {
+      target: { value: 'my dataset' },
+    });
+    userEvent.click(screen.getByRole('button', { name: /save/i }));
+
+    // `createStore` builds its reducer map at runtime, so state isn't typed.
+    const toasts = () =>
+      (
+        store.getState() as unknown as {
+          messageToasts: { toastType: string }[];
+        }
+      ).messageToasts;
+
+    await waitFor(() => {
+      expect(toasts()).toHaveLength(1);
+    });
+    expect(toasts()[0].toastType).toBe('DANGER_TOAST');
+    expect(onHide).not.toHaveBeenCalled();
+  });
+
   test('clearDatasetCache is imported and available', () => {
     const { clearDatasetCache } = require('src/utils/cachedSupersetGet');
 

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

@@ -61,6 +61,9 @@ import type Subject from 'src/types/Subject';
 import { openInNewTab, redirect } from 'src/utils/navigationUtils';
 import { mapSubjectValuesToIds } from 'src/features/subjects/SubjectPicker';
 
+// Derived so it can't drift from what `getClientErrorObject` accepts.
+type SaveErrorSource = Parameters<typeof getClientErrorObject>[0];
+
 interface QueryDatabase {
   id?: number;
 }
@@ -391,9 +394,18 @@ export const SaveDatasetModal = ({
         setDatasetName(getDefaultDatasetName());
         onHide();
       })
-      .catch(() => {
+      .catch((error?: SaveErrorSource) => {
         setLoading(false);
-        addDangerToast(t('An error occurred saving dataset'));
+        // `createDatasource` already toasted the server's message and rejects
+        // with nothing; only the chart-payload step needs its own.
+        if (!error) {
+          return;
+        }
+        getClientErrorObject(error).then(e =>
+          dispatch(
+            addDangerToast(e.error || t('An error occurred saving dataset')),
+          ),
+        );
       });
   };
 

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

@@ -27,6 +27,8 @@ import {
 import SaveQuery from 'src/SqlLab/components/SaveQuery';
 import { initialState, databases } from 'src/SqlLab/fixtures';
 
+const RESULT_COLUMNS = [{ column_name: 'col', type: 'STRING' }];
+
 const mockedProps = {
   queryEditorId: '123',
   animation: false,
@@ -35,7 +37,6 @@ const mockedProps = {
   onSave: () => {},
   saveQueryWarning: null,
   columns: [],
-  canSaveDataset: true,
 };
 
 const mockState = {
@@ -60,8 +61,31 @@ const splitSaveBtnProps = {
     ...mockedProps.database,
     allows_virtual_table_explore: true,
   },
+  columns: RESULT_COLUMNS,
 };
 
+const EDITOR_SQL = 'SELECT * FROM t';
+
+const stateWithLatestQuery = ({
+  id,
+  state,
+  sql = EDITOR_SQL,
+}: {
+  id: string;
+  state: string;
+  sql?: string;
+}) => ({
+  ...mockState,
+  sqlLab: {
+    ...mockState.sqlLab,
+    queryEditors: mockState.sqlLab.queryEditors.map(qe => ({
+      ...qe,
+      latestQueryId: id,
+    })),
+    queries: { [id]: { id, state, sql } },
+  },
+});
+
 const middlewares = [thunk];
 const mockStore = configureStore(middlewares);
 
@@ -97,6 +121,71 @@ describe('SavedQuery', () => {
     expect(saveBtn).toBeVisible();
   });
 
+  test('blocks "Save dataset" until the query has run successfully', () => {
+    // Without a successful run the save can only fail server-side.
+    render(<SaveQuery {...splitSaveBtnProps} />, {
+      useRedux: true,
+      store: mockStore(stateWithLatestQuery({ id: 'qid-1', state: 'failed' })),
+    });
+
+    expect(
+      screen.getByRole('button', { name: /save dataset/i }),
+    ).toBeDisabled();
+    // Saving the query itself is unaffected.
+    expect(screen.getByRole('button', { name: 'Save' })).toBeEnabled();
+  });
+
+  test('blocks "Save dataset" when no query has been run at all', () => {
+    render(<SaveQuery {...splitSaveBtnProps} />, {
+      useRedux: true,
+      store: mockStore(mockState),
+    });
+
+    expect(
+      screen.getByRole('button', { name: /save dataset/i }),
+    ).toBeDisabled();
+  });
+
+  test('blocks "Save dataset" when the SQL changed after a successful run', () => {
+    // The run succeeded, but not for what is in the editor now -- and it is
+    // the editor's SQL that gets saved.
+    render(<SaveQuery {...splitSaveBtnProps} />, {
+      useRedux: true,
+      store: mockStore(
+        stateWithLatestQuery({
+          id: 'qid-1',
+          state: 'success',
+          sql: 'SELECT 1 AS ran_earlier',
+        }),
+      ),
+    });
+
+    expect(
+      screen.getByRole('button', { name: /save dataset/i }),
+    ).toBeDisabled();
+  });
+
+  test('blocks "Save dataset" when the successful query returned no columns', () => {
+    // e.g. a DDL/DML statement -- there is nothing to introspect into a dataset.
+    render(<SaveQuery {...splitSaveBtnProps} columns={[]} />, {
+      useRedux: true,
+      store: mockStore(stateWithLatestQuery({ id: 'qid-1', state: 'success' })),
+    });
+
+    expect(
+      screen.getByRole('button', { name: /save dataset/i }),
+    ).toBeDisabled();
+  });
+
+  test('enables "Save dataset" once the query has succeeded', () => {
+    render(<SaveQuery {...splitSaveBtnProps} />, {
+      useRedux: true,
+      store: mockStore(stateWithLatestQuery({ id: 'qid-1', state: 'success' })),
+    });
+
+    expect(screen.getByRole('button', { name: /save dataset/i })).toBeEnabled();
+  });
+
   test('renders a save query modal when user clicks save button', () => {
     render(<SaveQuery {...mockedProps} />, {
       useRedux: true,
@@ -234,7 +323,7 @@ describe('SavedQuery', () => {
   test('renders a save dataset modal when user clicks "save dataset" menu item', async () => {
     render(<SaveQuery {...splitSaveBtnProps} />, {
       useRedux: true,
-      store: mockStore(mockState),
+      store: mockStore(stateWithLatestQuery({ id: 'qid-1', state: 'success' })),
     });
 
     const saveDatasetMenuItem = await screen.findByLabelText(/save dataset/i);
@@ -248,7 +337,7 @@ describe('SavedQuery', () => {
   test('renders the save dataset modal UI', async () => {
     render(<SaveQuery {...splitSaveBtnProps} />, {
       useRedux: true,
-      store: mockStore(mockState),
+      store: mockStore(stateWithLatestQuery({ id: 'qid-1', state: 'success' })),
     });
     const saveDatasetMenuItem = await screen.findByLabelText(/save dataset/i);
     userEvent.click(saveDatasetMenuItem);

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

@@ -17,6 +17,8 @@
  * under the License.
  */
 import { useState, useEffect, useMemo, ChangeEvent } from 'react';
+import { useSelector } from 'react-redux';
+import { Query, QueryState } from '@superset-ui/core';
 import type { DatabaseObject } from 'src/features/databases/types';
 import { t } from '@apache-superset/core/translation';
 import { styled } from '@apache-superset/core/theme';
@@ -37,7 +39,7 @@ import {
 } from 'src/SqlLab/components/SaveDatasetModal';
 import { getDatasourceAsSaveableDataset } from 'src/utils/datasourceUtils';
 import useQueryEditor from 'src/SqlLab/hooks/useQueryEditor';
-import { QueryEditor } from 'src/SqlLab/types';
+import { QueryEditor, SqlLabRootState } from 'src/SqlLab/types';
 import useLogAction from 'src/logger/useLogAction';
 import {
   LOG_ACTIONS_SQLLAB_CREATE_CHART,
@@ -52,7 +54,6 @@ interface SaveQueryProps {
   onUpdate: (arg0: QueryPayload, id: string) => void;
   saveQueryWarning: string | null;
   database: Partial<DatabaseObject> | undefined;
-  canSaveDataset: boolean;
 }
 
 export type QueryPayload = {
@@ -82,7 +83,6 @@ const SaveQuery = ({
   saveQueryWarning,
   database,
   columns,
-  canSaveDataset,
 }: SaveQueryProps) => {
   const queryEditor = useQueryEditor(queryEditorId, [
     'autorun',
@@ -113,6 +113,17 @@ const SaveQuery = ({
   const [label, setLabel] = useState<string>(defaultLabel);
   const [showSave, setShowSave] = useState<boolean>(false);
   const [showSaveDatasetModal, setShowSaveDatasetModal] = useState(false);
+  // Saving a dataset runs the SQL to introspect columns, so it needs a
+  // successful run of the SQL being saved that produced at least one column
+  // -- editing after a run invalidates it, and running a selection only
+  // validates that selection.
+  const latestQuery = useSelector<SqlLabRootState, Query | undefined>(
+    ({ sqlLab }) => sqlLab.queries[queryEditor.latestQueryId || ''],
+  );
+  const canSaveDataset =
+    latestQuery?.state === QueryState.Success &&
+    latestQuery.sql === queryEditor.sql &&
+    columns.length > 0;
   const isSaved = !!query.remoteId;
   const isLabelEmpty = label.trim().length === 0;
   const canExploreDatabase = !!database?.allows_virtual_table_explore;

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

@@ -356,7 +356,10 @@ describe('SqlEditor', () => {
     );
 
   test('enables the save dataset button when the latest query succeeded', async () => {
-    const { findByRole } = setupWithLatestQuery({ state: QueryState.Success });
+    const { findByRole } = setupWithLatestQuery({
+      state: QueryState.Success,
+      sql: mockedProps.queryEditor.sql,
+    });
     expect(await findByRole('button', { name: 'Save dataset' })).toBeEnabled();
   });
 

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

@@ -868,7 +868,6 @@ const SqlEditor: FC<Props> = ({
           }
           saveQueryWarning={saveQueryWarning}
           database={database}
-          canSaveDataset={successful && resultColumns.length > 0}
         />
         <ShareSqlLabQuery queryEditorId={queryEditor.id} />
       </>

superset-frontend/src/dashboard/util/permissionUtils.test.ts

@@ -259,6 +259,21 @@ describe('isUserEditorOrAdmin', () => {
   test('returns false when editors is omitted', () => {
     expect(isUserEditorOrAdmin(outsiderUser)).toEqual(false);
   });
+
+  test('returns true when the user is granted editorship only through extra_editors', () => {
+    expect(isUserEditorOrAdmin(editorUser, [], [10])).toEqual(true);
+  });
+
+  test('unions editors and extra_editors rather than preferring one', () => {
+    const nonMatchingSubject: Subject = { id: 999, label: 'Other', type: 1 };
+    expect(isUserEditorOrAdmin(editorUser, [nonMatchingSubject], [10])).toEqual(
+      true,
+    );
+  });
+
+  test('returns false when extra_editors names other subjects', () => {
+    expect(isUserEditorOrAdmin(editorUser, [], [999])).toEqual(false);
+  });
 });
 
 // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks

superset-frontend/src/dashboard/util/permissionUtils.ts

@@ -55,9 +55,6 @@ export const isUserInSubjects = (
   );
 };
 
-const isUserInEditors = (editors: Subject[] = []): boolean =>
-  isUserInSubjects(editors);
-
 export const isUserAdmin = (
   user?: UserWithPermissionsAndRoles | UndefinedUser,
 ) =>
@@ -66,10 +63,12 @@ export const isUserAdmin = (
     role => role.toLowerCase() === ADMIN_ROLE_NAME.toLowerCase(),
   );
 
+/** `extraEditors` is editorship granted via a deployment's EXTRA_EDITORS_RESOLVER. */
 export const isUserEditorOrAdmin = (
   user?: UserWithPermissionsAndRoles | UndefinedUser,
   editors: Subject[] = [],
-): boolean => isUserInEditors(editors) || isUserAdmin(user);
+  extraEditors?: SubjectRef[] | null,
+): boolean => isUserInSubjects(editors, extraEditors) || isUserAdmin(user);
 
 /**
  * Editorship of *dashboard*, matching the server's `is_editor`: the explicit

superset-frontend/src/features/charts/ChartCard.tsx

@@ -96,7 +96,11 @@ export default function ChartCard({
   const canEdit = hasPerm('can_write');
   const canDelete = hasPerm('can_write');
   const canExport = hasPerm('can_export');
-  const allowEdit = isUserEditorOrAdmin(user, chart.editors);
+  const allowEdit = isUserEditorOrAdmin(
+    user,
+    chart.editors,
+    chart.extra_editors,
+  );
   const menuItems: MenuItem[] = [];
 
   if (canEdit) {

superset-frontend/src/features/dashboards/DashboardCard.tsx

@@ -83,7 +83,11 @@ function DashboardCard({
   const canEdit = hasPerm('can_write');
   const canDelete = hasPerm('can_write');
   const canExport = hasPerm('can_export');
-  const allowEdit = isUserEditorOrAdmin(user, dashboard.editors);
+  const allowEdit = isUserEditorOrAdmin(
+    user,
+    dashboard.editors,
+    dashboard.extra_editors,
+  );
   const digest = dashboard.changed_on_utc || dashboard.changed_on;
   const thumbnailUrl =
     isFeatureEnabled(FeatureFlag.Thumbnails) && dashboard.id && digest

superset-frontend/src/pages/ChartList/index.tsx

@@ -650,7 +650,11 @@ function ChartList(props: ChartListProps) {
       },
       {
         Cell: ({ row: { original } }: CellProps<Chart>) => {
-          const allowEdit = isUserEditorOrAdmin(user, original.editors);
+          const allowEdit = isUserEditorOrAdmin(
+            user,
+            original.editors,
+            original.extra_editors,
+          );
           const openEditModal = () => openChartEditModal(original);
           const handleExport = () => handleBulkChartExport([original]);
           if (!canEdit && !canDelete && !canExport) {

superset-frontend/src/pages/DashboardList/index.tsx

@@ -122,6 +122,8 @@ export interface Dashboard {
   description?: string;
   thumbnail_url?: string | null;
   editors?: Subject[];
+  // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
+  extra_editors?: number[];
   viewers?: Subject[];
   tags: TagType[];
   created_by: object;
@@ -505,7 +507,11 @@ function DashboardList(props: DashboardListProps) {
       },
       {
         Cell: ({ row: { original } }: CellProps<Dashboard>) => {
-          const allowEdit = isUserEditorOrAdmin(user, original.editors);
+          const allowEdit = isUserEditorOrAdmin(
+            user,
+            original.editors,
+            original.extra_editors,
+          );
           const handleDelete = () =>
             handleDashboardDelete(
               original,

superset-frontend/src/types/Chart.ts

@@ -45,6 +45,8 @@ export interface Chart {
   cache_timeout: number | null;
   thumbnail_url?: string;
   editors?: Subject[];
+  // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
+  extra_editors?: number[];
   viewers?: Subject[];
   tags?: TagType[];
   last_saved_at?: string;

superset-frontend/src/views/CRUD/types.ts

@@ -67,6 +67,8 @@ export interface Dashboard {
   url: string;
   thumbnail_url?: string | null;
   editors?: Subject[];
+  // Bare subject ids from a deployment's EXTRA_EDITORS_RESOLVER.
+  extra_editors?: number[];
   viewers?: Subject[];
   loading?: boolean;
 }

superset/charts/api.py

@@ -92,7 +92,10 @@ from superset.exceptions import (
 )
 from superset.extensions import event_logger, security_manager
 from superset.models.slice import Slice
-from superset.security.manager import get_extra_editor_subject_ids
+from superset.security.manager import (
+    get_extra_editor_subject_ids,
+    get_extra_editors_by_pk,
+)
 from superset.subjects.filters import (
     FilterRelatedSubjects,
     subject_type_filter,
@@ -410,6 +413,15 @@ class ChartRestApi(SoftDeleteApiMixin, BaseSupersetModelRestApi):
         except ChartNotFoundError:
             return self.response_404()
 
+    def pre_get_list(self, data: dict[str, Any]) -> None:
+        """Attach ``extra_editors`` to each row, matching the single-object GET."""
+        super().pre_get_list(data)
+        ids = data.get("ids", [])
+        extra_editors_by_id = get_extra_editors_by_pk(Slice, ids)
+        for row, row_id in zip(data.get("result", []), ids, strict=False):
+            if row_id in extra_editors_by_id:
+                row["extra_editors"] = extra_editors_by_id[row_id]
+
     @expose("/<pk>/deck_layers/", methods=("GET",))
     @protect()
     @safe

superset/commands/dataset/create.py

@@ -33,7 +33,11 @@ from superset.commands.dataset.exceptions import (
 )
 from superset.commands.utils import populate_subjects
 from superset.daos.dataset import DatasetDAO
-from superset.exceptions import SupersetParseError, SupersetSecurityException
+from superset.exceptions import (
+    SupersetException,
+    SupersetParseError,
+    SupersetSecurityException,
+)
 from superset.extensions import security_manager
 from superset.sql.parse import Table
 from superset.utils.decorators import on_error, transaction
@@ -50,7 +54,25 @@ class CreateDatasetCommand(CreateMixin, BaseCommand):
         self.validate()
 
         dataset = DatasetDAO.create(attributes=self._properties)
-        dataset.fetch_metadata()
+        try:
+            dataset.fetch_metadata()
+        except SupersetException as ex:
+            # Not a SQLAlchemyError, so ``on_error`` re-raises it untouched and
+            # it escapes to FAB's ``@safe`` as an opaque 500 "Fatal error".
+            # Deliberately covers the 403 ``SupersetSecurityException`` raised
+            # for mutation/multi-statement SQL too: ``validate()`` already
+            # reports that class of rejection as a 422 on ``sql`` via
+            # ``DatasetDataAccessIsNotAllowed``.
+            raise DatasetInvalidError(
+                exceptions=[
+                    ValidationError(
+                        # ``lazy_gettext`` messages aren't ``str``, so
+                        # marshmallow won't wrap them into a list on its own.
+                        [str(ex.message)],
+                        field_name="sql" if self._properties.get("sql") else "table",
+                    )
+                ]
+            ) from ex
         return dataset
 
     def validate(self) -> None:  # noqa: C901

superset/commands/tag/create.py

@@ -18,8 +18,6 @@ import logging
 from functools import partial
 from typing import Any
 
-from jinja2.exceptions import TemplateError
-
 from superset import security_manager
 from superset.commands.base import BaseCommand, CreateMixin
 from superset.commands.tag.exceptions import TagCreateFailedError, TagInvalidError
@@ -29,7 +27,7 @@ from superset.commands.tag.utils import (
     to_object_type,
 )
 from superset.daos.tag import TagDAO
-from superset.exceptions import SupersetParseError, SupersetSecurityException
+from superset.exceptions import SupersetSecurityException
 from superset.tags.models import ObjectType, TagType
 from superset.utils.decorators import on_error, transaction
 
@@ -100,30 +98,8 @@ class CreateCustomTagCommand(CreateMixin, BaseCommand):
                     )
                 )
         except SupersetSecurityException:
-            # A routine, expected authorization denial; swallowed silently by
-            # design (no logging) and surfaced to the caller as a validation
-            # failure rather than an unhandled 500.
             exceptions.append(
-                TagCreateFailedError(
-                    f"Could not validate access for {object_type} {object_id}"
-                )
-            )
-        except (TemplateError, SupersetParseError) as ex:
-            # Authorizing a saved query parses its Jinja-templated SQL to resolve
-            # table references. Malformed Jinja (TemplateError) or an
-            # unresolvable partition macro (SupersetParseError) is a validation
-            # failure, not an unhandled 500 -- but unlike an access denial it is
-            # genuinely unexpected, so log it for server-side visibility and
-            # preserve the underlying error text instead of discarding it.
-            logger.warning(
-                "Could not parse query %s while validating tag access: %s",
-                object_id,
-                str(ex),
-            )
-            exceptions.append(
-                TagCreateFailedError(
-                    f"Could not validate access for {object_type} {object_id}: {ex}"
-                )
+                TagCreateFailedError(f"Access denied for {object_type} {object_id}")
             )
 
 

superset/common/form_data_query_context.py

@@ -213,7 +213,9 @@ def orderby_from_form_data(
     # The drag-and-drop "sort by" control persists a list; the frontend unwraps it
     # with ``ensureIsArray(...)[0]`` (``plugin-chart-table/src/buildQuery.ts:67``).
     # Read raw, a list would nest inside ``orderby`` and fail the query.
-    raw_sort_metric = form_data.get("timeseries_limit_metric")
+    raw_sort_metric = form_data.get("series_limit_metric") or form_data.get(
+        "timeseries_limit_metric"
+    )
     sort_metric = (
         next(iter(as_list(raw_sort_metric)), None) if raw_sort_metric else None
     ) or (metrics[0] if form_data.get("sort_by_metric") else None)

superset/dashboards/api.py

@@ -142,7 +142,10 @@ from superset.extensions import event_logger, security_manager
 from superset.models.dashboard import Dashboard
 from superset.models.embedded_dashboard import EmbeddedDashboard
 from superset.security.guest_token import GuestUser
-from superset.security.manager import get_extra_editor_subject_ids
+from superset.security.manager import (
+    get_extra_editor_subject_ids,
+    get_extra_editors_by_pk,
+)
 from superset.subjects.filters import (
     FilterRelatedSubjects,
     subject_type_filter,
@@ -433,6 +436,15 @@ class DashboardRestApi(
         """
         return super().get_list(**kwargs)
 
+    def pre_get_list(self, data: dict[str, Any]) -> None:
+        """Attach ``extra_editors`` to each row, matching the single-object GET."""
+        super().pre_get_list(data)
+        ids = data.get("ids", [])
+        extra_editors_by_id = get_extra_editors_by_pk(Dashboard, ids)
+        for row, row_id in zip(data.get("result", []), ids, strict=False):
+            if row_id in extra_editors_by_id:
+                row["extra_editors"] = extra_editors_by_id[row_id]
+
     list_select_columns = list_columns + ["changed_on", "created_on", "changed_by_fk"]
     order_columns = [
         "changed_by.first_name",

superset/examples/birth_names.py

@@ -290,7 +290,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 groupby=["name"],
                 adhoc_filters=[gen_filter("gender", "girl")],
                 row_limit=50,
-                timeseries_limit_metric=metric,
+                series_limit_metric=metric,
                 metrics=[metric],
             ),
             editors=[],
@@ -321,7 +321,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 groupby=["name"],
                 adhoc_filters=[gen_filter("gender", "boy")],
                 row_limit=50,
-                timeseries_limit_metric=metric,
+                series_limit_metric=metric,
                 metrics=[metric],
             ),
             editors=[],
@@ -498,7 +498,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 viz_type="echarts_timeseries_line",
                 granularity_sqla="ds",
                 groupby=["name"],
-                timeseries_limit_metric={
+                series_limit_metric={
                     "expressionType": "SIMPLE",
                     "column": {
                         "column_name": "num_california",
@@ -522,7 +522,7 @@ def create_slices(tbl: SqlaTable) -> tuple[list[Slice], list[Slice]]:
                 metrics=metrics,
                 groupby=["name"],
                 row_limit=50,
-                timeseries_limit_metric={
+                series_limit_metric={
                     "expressionType": "SIMPLE",
                     "column": {
                         "column_name": "num_california",

superset/examples/usa_births_names/charts/Boys.yaml

@@ -36,8 +36,8 @@ params:
   metrics:
   - sum__num
   row_limit: 50
+  series_limit_metric: sum__num
   time_range: '100 years ago : now'
-  timeseries_limit_metric: sum__num
   viz_type: table
 query_context: null
 slice_name: Boys

superset/examples/usa_births_names/charts/Girls.yaml

@@ -36,8 +36,8 @@ params:
   metrics:
   - sum__num
   row_limit: 50
+  series_limit_metric: sum__num
   time_range: '100 years ago : now'
-  timeseries_limit_metric: sum__num
   viz_type: table
 query_context: null
 slice_name: Girls

superset/security/manager.py

@@ -170,6 +170,36 @@ def get_extra_editor_subject_ids(resource: Model) -> list[int]:
     return subject_ids
 
 
+def get_extra_editors_by_pk(
+    model_cls: type[Model], primary_keys: list[Any]
+) -> dict[Any, list[int]]:
+    """
+    Resolve extra editor subject IDs for a batch of resources, keyed by
+    primary key. List responses only have serialized rows, not model
+    instances, so this re-queries the page's rows in one batched query.
+    """
+    if not primary_keys or not (
+        has_app_context() and current_app.config.get("EXTRA_EDITORS_RESOLVER")
+    ):
+        return {}
+
+    # pylint: disable=import-outside-toplevel
+    from superset import db
+    from superset.models.helpers import SKIP_VISIBILITY_FILTER_CLASSES
+
+    pk_col = inspect(model_cls).primary_key[0]
+    resources = (
+        db.session.query(model_cls)
+        .execution_options(**{SKIP_VISIBILITY_FILTER_CLASSES: {model_cls}})
+        .filter(pk_col.in_(primary_keys))
+        .all()
+    )
+    return {
+        getattr(resource, pk_col.name): get_extra_editor_subject_ids(resource)
+        for resource in resources
+    }
+
+
 def _render_permission_instructions_link(
     *,
     datasource_id: str = "",

tests/integration_tests/dashboards/api_tests.py

@@ -922,6 +922,55 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
             db.session.delete(dashboard)
             db.session.commit()
 
+    def test_get_dashboards_list_omits_extra_editors_by_default(self):
+        """No EXTRA_EDITORS_RESOLVER configured: list rows omit extra_editors."""
+        admin = self.get_user("admin")
+        dashboard = self.insert_dashboard(
+            "no_extra_editors_list_dashboard",
+            "no-extra-editors-list-dashboard",
+            [admin.id],
+        )
+        try:
+            self.login(ADMIN_USERNAME)
+            rv = self.client.get("api/v1/dashboard/")
+            assert rv.status_code == 200
+            data = json.loads(rv.data.decode("utf-8"))
+            row = next(
+                d
+                for d in data["result"]
+                if d["dashboard_title"] == dashboard.dashboard_title
+            )
+            assert "extra_editors" not in row
+        finally:
+            db.session.delete(dashboard)
+            db.session.commit()
+
+    @with_config({"EXTRA_EDITORS_RESOLVER": lambda resource: [123]})
+    def test_get_dashboards_list_includes_extra_editors_when_resolver_configured(
+        self,
+    ):
+        """List rows get extra_editors too, mirroring the single-object GET."""
+        admin = self.get_user("admin")
+        dashboard = self.insert_dashboard(
+            "extra_editors_list_dashboard",
+            "extra-editors-list-dashboard",
+            [admin.id],
+        )
+        try:
+            self.login(ADMIN_USERNAME)
+            rv = self.client.get("api/v1/dashboard/")
+            assert rv.status_code == 200
+            data = json.loads(rv.data.decode("utf-8"))
+            row = next(
+                d
+                for d in data["result"]
+                if d["dashboard_title"] == dashboard.dashboard_title
+            )
+            assert row["extra_editors"] == [123]
+        finally:
+            db.session.delete(dashboard)
+            db.session.commit()
+
     def test_get_charts_admin_sees_existing_charts(self):
         """Regression for #25890: GET /api/v1/chart/ as an Admin user should
         return existing charts, not an empty list."""
@@ -944,6 +993,41 @@ class TestDashboardApi(ApiEditorsTestCaseMixin, InsertChartMixin, SupersetTestCa
             db.session.delete(chart)
             db.session.commit()
 
+    def test_get_charts_list_omits_extra_editors_by_default(self):
+        """No EXTRA_EDITORS_RESOLVER configured: list rows omit extra_editors."""
+        admin = self.get_user("admin")
+        chart = self.insert_chart(
+            "no_extra_editors_list_chart", [admin.id], 1, params="{}"
+        )
+        try:
+            self.login(ADMIN_USERNAME)
+            rv = self.client.get("api/v1/chart/")
+            assert rv.status_code == 200
+            data = json.loads(rv.data.decode("utf-8"))
+            row = next(c for c in data["result"] if c["slice_name"] == chart.slice_name)
+            assert "extra_editors" not in row
+        finally:
+            db.session.delete(chart)
+            db.session.commit()
+
+    @with_config({"EXTRA_EDITORS_RESOLVER": lambda resource: [123]})
+    def test_get_charts_list_includes_extra_editors_when_resolver_configured(self):
+        """List rows get extra_editors too, mirroring the single-object GET."""
+        admin = self.get_user("admin")
+        chart = self.insert_chart(
+            "extra_editors_list_chart", [admin.id], 1, params="{}"
+        )
+        try:
+            self.login(ADMIN_USERNAME)
+            rv = self.client.get("api/v1/chart/")
+            assert rv.status_code == 200
+            data = json.loads(rv.data.decode("utf-8"))
+            row = next(c for c in data["result"] if c["slice_name"] == chart.slice_name)
+            assert row["extra_editors"] == [123]
+        finally:
+            db.session.delete(chart)
+            db.session.commit()
+
     def test_get_dashboards_filter(self):
         """
         Dashboard API: Test get dashboards filter

tests/unit_tests/commands/dataset/test_create.py

@@ -18,11 +18,15 @@ from unittest.mock import Mock, patch
 
 import pytest
 from marshmallow import ValidationError
+from pytest_mock import MockerFixture
 
 from superset.commands.dataset.create import CreateDatasetCommand
 from superset.commands.dataset.exceptions import DatasetInvalidError
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
-from superset.exceptions import SupersetParseError
+from superset.exceptions import (
+    SupersetGenericDBErrorException,
+    SupersetParseError,
+)
 from superset.models.core import Database
 
 
@@ -250,3 +254,83 @@ def test_create_dataset_generic_exists_error_when_no_twin() -> None:
                             )
                             with pytest.raises(DatasetInvalidError):
                                 command.validate()
+
+
+def test_create_dataset_metadata_fetch_error_is_structured(
+    mocker: MockerFixture,
+) -> None:
+    """A metadata-fetch failure must surface the engine's own message.
+
+    ``run()`` executes the SQL to introspect columns; the resulting
+    ``SupersetGenericDBErrorException`` used to escape as a 500 "Fatal error".
+    """
+    mocker.patch.object(CreateDatasetCommand, "validate")
+    dataset = Mock()
+    dataset.fetch_metadata.side_effect = SupersetGenericDBErrorException(
+        message="Invalid SQL: Unable to parse: SELECT ...",
+    )
+    mocker.patch(
+        "superset.commands.dataset.create.DatasetDAO.create",
+        return_value=dataset,
+    )
+
+    command = CreateDatasetCommand(
+        {
+            "database": 1,
+            "table_name": "dataset wrong",
+            "sql": "SELECT ...",
+        }
+    )
+
+    with pytest.raises(DatasetInvalidError) as exc_info:
+        command.run()
+
+    validation_errors = exc_info.value._exceptions
+    assert len(validation_errors) == 1
+    assert validation_errors[0].field_name == "sql"
+    assert "Invalid SQL: Unable to parse: SELECT ..." in str(
+        validation_errors[0].messages[0]
+    )
+
+
+def test_create_dataset_metadata_fetch_error_physical_table(
+    mocker: MockerFixture,
+) -> None:
+    """The same conversion applies to physical datasets, keyed on ``table``."""
+    mocker.patch.object(CreateDatasetCommand, "validate")
+    dataset = Mock()
+    dataset.fetch_metadata.side_effect = SupersetGenericDBErrorException(
+        message="(psycopg2.OperationalError) could not connect to server",
+    )
+    mocker.patch(
+        "superset.commands.dataset.create.DatasetDAO.create",
+        return_value=dataset,
+    )
+
+    command = CreateDatasetCommand({"database": 1, "table_name": "physical_table"})
+
+    with pytest.raises(DatasetInvalidError) as exc_info:
+        command.run()
+
+    validation_errors = exc_info.value._exceptions
+    assert validation_errors[0].field_name == "table"
+    assert "could not connect to server" in str(validation_errors[0].messages[0])
+
+
+def test_create_dataset_run_succeeds_when_metadata_fetch_works(
+    mocker: MockerFixture,
+) -> None:
+    """Control: the happy path still returns the created dataset."""
+    mocker.patch.object(CreateDatasetCommand, "validate")
+    dataset = Mock()
+    mocker.patch(
+        "superset.commands.dataset.create.DatasetDAO.create",
+        return_value=dataset,
+    )
+
+    command = CreateDatasetCommand(
+        {"database": 1, "table_name": "good_dataset", "sql": "SELECT 1 AS a"}
+    )
+
+    assert command.run() is dataset
+    dataset.fetch_metadata.assert_called_once()

tests/unit_tests/common/test_form_data_query_context.py

@@ -218,6 +218,31 @@ def test_orderby_uses_timeseries_limit_metric_and_order_desc() -> None:
     assert query["orderby"] == [["revenue", True]]
 
 
+def test_orderby_uses_series_limit_metric_and_order_desc() -> None:
+    # series_limit_metric is the current field name; timeseries_limit_metric is
+    # the deprecated alias kept above for back-compat with old saved charts.
+    form_data = {
+        "metrics": ["count"],
+        "groupby": ["c"],
+        "series_limit_metric": "revenue",
+        "order_desc": False,
+    }
+    query = build_query_context_from_form_data(form_data, DATASOURCE)["queries"][0]
+    assert query["orderby"] == [["revenue", True]]
+
+
+def test_orderby_prefers_series_limit_metric_over_deprecated_alias() -> None:
+    form_data = {
+        "metrics": ["count"],
+        "groupby": ["c"],
+        "series_limit_metric": "revenue",
+        "timeseries_limit_metric": "profit",
+        "order_desc": False,
+    }
+    query = build_query_context_from_form_data(form_data, DATASOURCE)["queries"][0]
+    assert query["orderby"] == [["revenue", True]]
+
+
 def test_orderby_pie_sort_by_metric() -> None:
     form_data = {"metric": "count", "groupby": ["c"], "sort_by_metric": True}
     query = build_query_context_from_form_data(form_data, DATASOURCE, viz_type="pie")[

tests/unit_tests/datasets/api_tests.py

@@ -214,3 +214,46 @@ def test_handle_filters_args_returns_request_scoped_filters(
     fresh_filters = api.datamodel.get_filters.return_value
     assert fresh_filters.rest_add_filters.call_count == 2
     assert fresh_filters.get_joined_filters.call_count == 2
+
+
+def test_post_dataset_with_invalid_sql_returns_actionable_422(
+    session: Session,
+    client: Any,
+    full_api_access: None,
+) -> None:
+    """Saving a dataset over unrunnable SQL must explain what is wrong.
+
+    With blanket database access ``validate()`` never parses the SQL, so
+    ``run()``'s column introspection is the first thing to reject it. That
+    used to surface as a bare 500 ``{"message": "Fatal error"}``.
+    """
+    from superset.connectors.sqla.models import SqlaTable
+    from superset.models.core import Database
+
+    SqlaTable.metadata.create_all(db.session.get_bind())
+
+    database = Database(database_name="invalid_sql_db", sqlalchemy_uri="sqlite://")
+    db.session.add(database)
+    db.session.flush()
+
+    response = client.post(
+        "/api/v1/dataset/",
+        json={
+            "database": database.id,
+            "schema": "main",
+            "table_name": "dataset wrong",
+            "sql": "SELECT ...",
+        },
+    )
+
+    assert response.status_code == 422
+    message = response.json["message"]
+    assert "Fatal error" not in str(message)
+    # Not the parser's exact wording -- that would break on a sqlglot bump.
+    assert message["sql"][0].startswith("Invalid SQL")
+
+    # The failed create must not leave a half-built dataset behind.
+    assert (
+        db.session.query(SqlaTable).filter_by(table_name="dataset wrong").one_or_none()
+        is None
+    )

tests/unit_tests/tags/commands/create_test.py

@@ -108,75 +108,6 @@ def test_create_command_success(session_with_data: Session, mocker: MockerFixtur
         )
 
 
-def test_validate_object_access_query_malformed_jinja(
-    session_with_data: Session, mocker: MockerFixture
-):
-    """A saved query whose Jinja-templated SQL fails to parse during access
-    checks must surface as a validation error, not an unhandled
-    ``jinja2.TemplateError`` escaping as a 500.
-
-    When ``raise_for_access(query=...)`` authorizes a saved query via
-    per-table permissions it parses the query's Jinja SQL (e.g. an unclosed
-    ``{% if %}`` block raises ``TemplateSyntaxError``). Mock that call to raise
-    the ``TemplateError`` directly so the test stays hermetic and does not open
-    a live DB connection to introspect table-level perms.
-    """
-    from jinja2.exceptions import TemplateError
-
-    from superset.commands.tag.create import CreateCustomTagCommand
-    from superset.commands.tag.exceptions import TagInvalidError
-    from superset.models.sql_lab import SavedQuery
-    from superset.tags.models import ObjectType
-
-    query = db.session.query(SavedQuery).first()
-
-    mocker.patch("superset.commands.tag.create.to_object_model", return_value=query)
-    mocker.patch(
-        "superset.commands.tag.create.security_manager.raise_for_access",
-        side_effect=TemplateError("unclosed {% if %}"),
-    )
-
-    command = CreateCustomTagCommand(ObjectType.query, query.id, ["tag"])
-
-    with pytest.raises(TagInvalidError):
-        command.validate()
-
-
-def test_validate_object_access_query_unresolvable_partition_macro(
-    session_with_data: Session, mocker: MockerFixture
-):
-    """A saved query whose partition macro cannot be resolved statically raises
-    ``SupersetParseError`` during access checks. Like ``TemplateError``, it is a
-    sibling of ``SupersetSecurityException`` under ``SupersetErrorException`` and
-    would otherwise escape as an unhandled 500, so it must also surface as a
-    validation error.
-
-    Mock ``raise_for_access`` to raise the error directly so the test stays
-    hermetic and does not open a live DB connection to introspect table perms.
-    """
-    from superset.commands.tag.create import CreateCustomTagCommand
-    from superset.commands.tag.exceptions import TagInvalidError
-    from superset.exceptions import SupersetParseError
-    from superset.models.sql_lab import SavedQuery
-    from superset.tags.models import ObjectType
-
-    query = db.session.query(SavedQuery).first()
-
-    mocker.patch("superset.commands.tag.create.to_object_model", return_value=query)
-    mocker.patch(
-        "superset.commands.tag.create.security_manager.raise_for_access",
-        side_effect=SupersetParseError(
-            sql="select * from {{ latest_partition('foo') }}",
-            message="Unresolvable partition macro",
-        ),
-    )
-
-    command = CreateCustomTagCommand(ObjectType.query, query.id, ["tag"])
-
-    with pytest.raises(TagInvalidError):
-        command.validate()
-
-
 def test_create_command_success_clear(
     session_with_data: Session, mocker: MockerFixture
 ):