fix(sqllab): invalidate the dataset-save gate when the editor SQL changes

.github/labeler.yml

@@ -29,7 +29,7 @@
 "dependencies:python":
 - changed-files:
   - any-glob-to-any-file:
-    - 'superset/requirements/**'
+    - 'requirements/**'
     - 'superset/translations/requirements.txt'
     - 'RELEASING/requirements.txt'
 

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

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { render, screen } from 'spec/helpers/testing-library';
+import { render, screen, userEvent } from 'spec/helpers/testing-library';
 import SaveDatasetActionButton from 'src/SqlLab/components/SaveDatasetActionButton';
 
 // eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
@@ -41,4 +41,37 @@ describe('SaveDatasetActionButton', () => {
     expect(saveBtn).toBeVisible();
     expect(saveDatasetBtn).toBeVisible();
   });
+
+  test('disables only the dataset button when saveDatasetDisabled is set', () => {
+    const onSaveAsExplore = jest.fn();
+    render(
+      <SaveDatasetActionButton
+        setShowSave={() => true}
+        onSaveAsExplore={onSaveAsExplore}
+        saveDatasetDisabled
+      />,
+    );
+
+    // Saving the query needs no results.
+    expect(screen.getByRole('button', { name: 'Save' })).toBeEnabled();
+    expect(
+      screen.getByRole('button', { name: /save dataset/i }),
+    ).toBeDisabled();
+  });
+
+  test('explains why the dataset button is unavailable', async () => {
+    render(
+      <SaveDatasetActionButton
+        setShowSave={() => true}
+        onSaveAsExplore={jest.fn()}
+        saveDatasetDisabled
+      />,
+    );
+
+    userEvent.hover(screen.getByRole('button', { name: /save dataset/i }));
+
+    expect(
+      await screen.findByText('You must run the query successfully first'),
+    ).toBeInTheDocument();
+  });
 });

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

@@ -23,11 +23,14 @@ import { Button } from '@superset-ui/core/components';
 interface SaveDatasetActionButtonProps {
   setShowSave: (arg0: boolean) => void;
   onSaveAsExplore?: () => void;
+  /** Set while the query has not run successfully. */
+  saveDatasetDisabled?: boolean;
 }
 
 const SaveDatasetActionButton = ({
   setShowSave,
   onSaveAsExplore,
+  saveDatasetDisabled = false,
 }: SaveDatasetActionButtonProps) => (
   <>
     <Button
@@ -44,7 +47,12 @@ const SaveDatasetActionButton = ({
         variant="text"
         onClick={() => onSaveAsExplore?.()}
         icon={<Icons.TableOutlined />}
-        tooltip={t('Save or Overwrite Dataset')}
+        tooltip={
+          saveDatasetDisabled
+            ? t('You must run the query successfully first')
+            : t('Save or Overwrite Dataset')
+        }
+        disabled={saveDatasetDisabled}
         aria-label={t('Save dataset')}
       />
     )}

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

@@ -61,6 +61,28 @@ const splitSaveBtnProps = {
   },
 };
 
+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);
 
@@ -96,6 +118,59 @@ 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('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,
@@ -233,7 +308,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);
@@ -247,7 +322,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,
@@ -111,6 +113,15 @@ 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 -- 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 hasSuccessfulQuery =
+    latestQuery?.state === QueryState.Success &&
+    latestQuery.sql === queryEditor.sql;
   const isSaved = !!query.remoteId;
   const isLabelEmpty = label.trim().length === 0;
   const canExploreDatabase = !!database?.allows_virtual_table_explore;
@@ -207,6 +218,7 @@ const SaveQuery = ({
         <SaveDatasetActionButton
           setShowSave={setShowSave}
           onSaveAsExplore={canExploreDatabase ? onSaveAsExplore : undefined}
+          saveDatasetDisabled={!hasSuccessfulQuery}
         />
       )}
       <SaveDatasetModal

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/db_engine_specs/README.md

@@ -83,7 +83,7 @@ The tables below (generated via `python superset/db_engine_specs/lib.py`) summar
 | Databricks (legacy) | 70 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | StarRocks | 69 | Supported | Partial | Supported | Partial | Partial | Partial |
 | SingleStore | 68 | Supported | Partial | Supported | Not supported | Partial | Not supported |
-| ClickHouse Connect (Superset) | 61 | Supported | Partial | Partial | Partial | Partial | Not supported |
+| ClickHouse Connect (Superset) | 62 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | Google Sheets | 61 | Supported | Partial | Supported | Supported | Partial | Partial |
 | Aurora MySQL (Data API) | 59 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | MariaDB | 59 | Supported | Partial | Supported | Partial | Partial | Not supported |
@@ -91,7 +91,7 @@ The tables below (generated via `python superset/db_engine_specs/lib.py`) summar
 | OceanBase | 59 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | MotherDuck | 58 | Supported | Partial | Supported | Not supported | Partial | Not supported |
 | KustoSQL | 54 | Supported | Partial | Supported | Partial | Partial | Not supported |
-| ClickHouse | 51 | Supported | Partial | Partial | Partial | Partial | Not supported |
+| ClickHouse | 52 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | Databend | 51 | Supported | Partial | Supported | Partial | Partial | Not supported |
 | Apache Drill | 50 | Supported | Partial | Supported | Partial | Partial | Partial |
 | Apache Druid | 47 | Partial | Partial | Supported | Partial | Partial | Not supported |
@@ -293,8 +293,8 @@ The tables below (generated via `python superset/db_engine_specs/lib.py`) summar
 | Aurora MySQL (Data API) | True | True | True | True | True | True | True | True |
 | Aurora PostgreSQL (Data API) | True | True | True | True | True | True | True | True |
 | Azure Synapse | True | True | True | True | True | True | True | True |
-| ClickHouse | False | True | True | True | True | True | True | True |
-| ClickHouse Connect (Superset) | False | True | True | True | True | True | True | True |
+| ClickHouse | True | True | True | True | True | True | True | True |
+| ClickHouse Connect (Superset) | True | True | True | True | True | True | True | True |
 | CockroachDB | True | True | True | True | True | True | True | True |
 | Couchbase | True | True | True | True | False | True | True | True |
 | CrateDB | True | True | True | True | True | True | True | True |

superset/db_engine_specs/clickhouse.py

@@ -112,6 +112,7 @@ class ClickHouseBaseEngineSpec(BaseEngineSpec):
 
     _time_grain_expressions = {
         None: "{col}",
+        "PT1S": "toStartOfSecond(toDateTime64({col}, 3))",
         "PT1M": "toStartOfMinute(toDateTime({col}))",
         "PT5M": "toDateTime(intDiv(toUInt32(toDateTime({col})), 300)*300)",
         "PT10M": "toDateTime(intDiv(toUInt32(toDateTime({col})), 600)*600)",

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/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/db_engine_specs/test_clickhouse.py

@@ -62,6 +62,20 @@ def test_convert_dttm(
     assert_convert_dttm(spec, target_type, expected_result, dttm)
 
 
+@pytest.mark.parametrize(
+    "time_grain,expected",
+    [
+        (None, "{col}"),
+        ("PT1S", "toStartOfSecond(toDateTime64({col}, 3))"),
+        ("PT1M", "toStartOfMinute(toDateTime({col}))"),
+    ],
+)
+def test_time_grain_expressions(time_grain: Optional[str], expected: str) -> None:
+    from superset.db_engine_specs.clickhouse import ClickHouseBaseEngineSpec
+
+    assert ClickHouseBaseEngineSpec._time_grain_expressions[time_grain] == expected
+
+
 def test_convert_dttm_normalizes_aware_datetime_to_utc() -> None:
     from superset.db_engine_specs.clickhouse import (
         ClickHouseEngineSpec as spec,  # noqa: N813