test: mock ChartDataCommand.run for the ChartDataQueryFailedError no-reraise test (SC-118140)

Automated PR review (bito-code-review) correctly flagged that this test
mocked the schema loader's side effect instead of ChartDataCommand.run,
so it never actually reached the run() call the exception is meant to
simulate failing at. The except clause still catches the exception either
way (same try block), so the assertion was never wrong, but mocking at
the real trigger point matches the sibling ChartDataCacheLoadError test
and the corrected integration test, and is more representative of the
actual failure path.

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

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",
@@ -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": {
@@ -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",
@@ -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/components/Datasource/components/DatasourceEditor/DatasourceEditor.tsx

@@ -779,35 +779,6 @@ function EditorsSelector({
 const ResultTable =
   extensionsRegistry.get('sqleditor.extension.resultTable') ?? FilterableTable;
 
-// D3's '%' type is a valid spec that multiplies by 100, so it never trips
-// the "Invalid format" fallback even when applied to a raw count.
-const isPercentD3Format = (d3format?: string): boolean =>
-  !!d3format && d3format.trim().endsWith('%');
-
-const isCountExpression = (expression?: string): boolean =>
-  !!expression && /^\s*count\s*\(/i.test(expression);
-
-function renderMetricFormatWarning(item: Record<string, any>): ReactNode {
-  if (
-    !isCountExpression(item.expression) ||
-    !isPercentD3Format(item.d3format)
-  ) {
-    return null;
-  }
-  return (
-    <Alert
-      css={themeParam => ({ marginBottom: themeParam.sizeUnit * 4 })}
-      type="warning"
-      showIcon
-      message={t(
-        'This metric is a count, but its D3 format is a percentage. ' +
-          'Percent formats multiply the value by 100, which will make a ' +
-          'raw count render as a misleadingly large number.',
-      )}
-    />
-  );
-}
-
 // Redux connector types
 interface QueryPayload {
   client_id?: string;
@@ -2169,7 +2140,7 @@ function DatasourceEditor({
           }}
           expandFieldset={
             <FormContainer>
-              <Fieldset compact renderWarning={renderMetricFormatWarning}>
+              <Fieldset compact>
                 <Field
                   fieldKey="expression"
                   label={t('SQL expression')}

superset-frontend/src/components/Datasource/components/DatasourceEditor/tests/DatasourceEditorMetricFormatWarning.test.tsx

@@ -1,91 +0,0 @@
-/**
- * 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 { screen, userEvent, waitFor } from 'spec/helpers/testing-library';
-import {
-  createProps,
-  DATASOURCE_ENDPOINT,
-  setupDatasourceEditorMocks,
-  cleanupAsyncOperations,
-  fastRender,
-  dismissDatasourceWarning,
-} from './DatasourceEditor.test.utils';
-
-beforeEach(() => {
-  fetchMock.get(DATASOURCE_ENDPOINT, [], { name: DATASOURCE_ENDPOINT });
-  setupDatasourceEditorMocks();
-});
-
-afterEach(async () => {
-  await cleanupAsyncOperations();
-  fetchMock.clearHistory().removeRoutes();
-});
-
-const WARNING_TEXT = /D3 format is a percentage/i;
-
-// A '%' format is valid syntax, so it never hits the "Invalid format" fallback.
-test('warns when a percent D3 format is set on a COUNT metric', async () => {
-  const testProps = createProps();
-  fastRender(testProps);
-  await dismissDatasourceWarning();
-
-  await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
-  const expandToggles = await screen.findAllByLabelText(/expand row/i);
-  // Rows sort by metric id descending, so `COUNT(*)` (id 7) is first.
-  await userEvent.click(expandToggles[0]);
-
-  expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument();
-
-  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), '.0%');
-
-  expect(await screen.findByText(WARNING_TEXT)).toBeInTheDocument();
-});
-
-test('does not warn for a non-percent format on a COUNT metric', async () => {
-  const testProps = createProps();
-  fastRender(testProps);
-  await dismissDatasourceWarning();
-
-  await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
-  const expandToggles = await screen.findAllByLabelText(/expand row/i);
-  await userEvent.click(expandToggles[0]);
-
-  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), ',.0f');
-
-  await waitFor(() =>
-    expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument(),
-  );
-});
-
-test('does not warn for a percent format on a non-COUNT metric', async () => {
-  const testProps = createProps();
-  fastRender(testProps);
-  await dismissDatasourceWarning();
-
-  await userEvent.click(await screen.findByTestId('collection-tab-Metrics'));
-  const expandToggles = await screen.findAllByLabelText(/expand row/i);
-  // Rows sort by metric id descending, so id 1 (`SUM(...)`) sorts last.
-  await userEvent.click(expandToggles[6]);
-
-  await userEvent.type(await screen.findByPlaceholderText('%y/%m/%d'), '.0%');
-
-  await waitFor(() =>
-    expect(screen.queryByText(WARNING_TEXT)).not.toBeInTheDocument(),
-  );
-});

superset-frontend/src/components/Datasource/components/Fieldset/index.tsx

@@ -28,7 +28,6 @@ export interface FieldsetProps {
   item?: Record<string, any>;
   title?: ReactNode;
   compact?: boolean;
-  renderWarning?: (item: Record<string, any>) => ReactNode;
 }
 
 type fieldKeyType = string | number;
@@ -39,7 +38,6 @@ export default function Fieldset({
   item = {},
   title = null,
   compact = false,
-  renderWarning,
 }: FieldsetProps) {
   // Controls report their edits asynchronously - TextControl debounces by
   // FAST_DEBOUNCE - so the callback that eventually fires was built during an
@@ -80,7 +78,6 @@ export default function Fieldset({
         </Typography.Title>
       )}
 
-      {renderWarning?.(item)}
       {recurseReactClone(children, Field, propExtender)}
     </Form>
   );

superset-frontend/src/explore/components/DataTablesPane/components/ResultsPaneOnDashboard.tsx

@@ -88,12 +88,7 @@ export const ResultsPaneOnDashboard = ({
 
   return (
     <Wrapper>
-      <Tabs
-        fullHeight
-        activeKey={activeTabKey}
-        onChange={setActiveTabKey}
-        items={items}
-      />
+      <Tabs activeKey={activeTabKey} onChange={setActiveTabKey} items={items} />
     </Wrapper>
   );
 };

superset/tasks/async_queries.py

@@ -106,6 +106,10 @@ def load_chart_data_into_cache(
 ) -> None:
     # pylint: disable=import-outside-toplevel
     from superset.commands.chart.data.get_data_command import ChartDataCommand
+    from superset.commands.chart.exceptions import (
+        ChartDataCacheLoadError,
+        ChartDataQueryFailedError,
+    )
 
     with override_user(_load_user_from_job_metadata(job_metadata), force=False):
         try:
@@ -123,6 +127,20 @@ def load_chart_data_into_cache(
         except SoftTimeLimitExceeded as ex:
             _handle_soft_time_limit(job_metadata, ex, "loading chart data")
             raise
+        except (ChartDataCacheLoadError, ChartDataQueryFailedError) as ex:
+            # These map to 422/400 in the synchronous chart/data endpoint (see
+            # ChartDataRestApi._get_data_response) - expected, client-facing
+            # validation failures (e.g. a chart still referencing columns a
+            # customer has since dropped from the dataset), not application
+            # bugs. The failure is already delivered to the client via
+            # update_job below; re-raising would only surface it a second
+            # time as an unhandled Celery task exception.
+            logger.info("Chart data query failed while loading into cache: %s", ex)
+            async_query_manager.update_job(
+                job_metadata,
+                async_query_manager.STATUS_ERROR,
+                errors=sanitize_error_dicts([{"message": str(ex.message)}]),
+            )
         except Exception as ex:
             # Extract SIP-40 style errors when available
             if isinstance(ex, SupersetErrorException):

tests/integration_tests/tasks/async_queries_tests.py

@@ -110,8 +110,11 @@ class TestAsyncQueries(SupersetTestCase):
             "status": "pending",
             "errors": [],
         }
-        with pytest.raises(ChartDataQueryFailedError):
-            load_chart_data_into_cache(job_metadata, query_context)
+        # ChartDataQueryFailedError mirrors the synchronous chart/data endpoint's
+        # 400 (see ChartDataRestApi._get_data_response) - an expected validation
+        # failure, not a bug, so the task reports it via update_job and does not
+        # re-raise (see superset/tasks/async_queries.py).
+        load_chart_data_into_cache(job_metadata, query_context)
 
         mock_run_command.assert_called_once_with(cache=True)
         errors = [{"message": "Error: foo"}]

tests/unit_tests/tasks/test_async_queries.py

@@ -21,7 +21,10 @@ import pytest
 from celery.exceptions import SoftTimeLimitExceeded
 from flask_babel import lazy_gettext as _
 
-from superset.commands.chart.exceptions import ChartDataQueryFailedError
+from superset.commands.chart.exceptions import (
+    ChartDataCacheLoadError,
+    ChartDataQueryFailedError,
+)
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.exceptions import (
     OAuth2RedirectError,
@@ -43,7 +46,7 @@ def test_load_chart_data_into_cache_with_error(
     job_metadata = {"user_id": 1}
     form_data = {}
     err_message = "Something went wrong"
-    err = ChartDataQueryFailedError(_(err_message))
+    err = RuntimeError(err_message)
 
     mock_user = mock.MagicMock()
     mock_query_context_schema = mock.MagicMock()
@@ -54,7 +57,7 @@ def test_load_chart_data_into_cache_with_error(
 
     mock_query_context_schema.load.side_effect = err
 
-    with pytest.raises(ChartDataQueryFailedError):
+    with pytest.raises(RuntimeError):
         load_chart_data_into_cache(job_metadata, form_data)
 
     expected_errors = [{"message": err_message}]
@@ -64,6 +67,76 @@ def test_load_chart_data_into_cache_with_error(
     )
 
 
+@mock.patch("superset.tasks.async_queries.security_manager")
+@mock.patch("superset.tasks.async_queries.async_query_manager")
+@mock.patch("superset.commands.chart.data.get_data_command.ChartDataCommand")
+@mock.patch("superset.tasks.async_queries.ChartDataQueryContextSchema")
+def test_load_chart_data_into_cache_with_query_failed_error_does_not_reraise(
+    mock_query_context_schema_cls: mock.MagicMock,
+    mock_command_cls: mock.MagicMock,
+    mock_async_query_manager: mock.MagicMock,
+    mock_security_manager: mock.MagicMock,
+) -> None:
+    """
+    ChartDataQueryFailedError maps to a 400 in the synchronous chart/data
+    endpoint (see ChartDataRestApi._get_data_response) - an expected,
+    client-facing validation failure (e.g. a chart still referencing columns
+    a customer has since dropped from the dataset), not an application bug.
+    The task must still report it to the client via update_job, but must not
+    re-raise it - that would surface it a second time as an unhandled Celery
+    task exception.
+    """
+    from superset.tasks.async_queries import load_chart_data_into_cache
+
+    job_metadata = {"user_id": 1}
+    form_data: dict[str, Any] = {}
+    err_message = "Columns missing in dataset: ['foo']"
+
+    mock_security_manager.get_user_by_id.return_value = mock.MagicMock()
+    mock_async_query_manager.STATUS_ERROR = "error"
+    mock_query_context_schema_cls.return_value.load.return_value = mock.MagicMock()
+    mock_command_cls.return_value.run.side_effect = ChartDataQueryFailedError(
+        _(err_message)
+    )
+
+    # Should not raise.
+    load_chart_data_into_cache(job_metadata, form_data)
+
+    mock_async_query_manager.update_job.assert_called_once_with(
+        job_metadata, "error", errors=[{"message": err_message}]
+    )
+
+
+@mock.patch("superset.tasks.async_queries.security_manager")
+@mock.patch("superset.tasks.async_queries.async_query_manager")
+@mock.patch("superset.commands.chart.data.get_data_command.ChartDataCommand")
+@mock.patch("superset.tasks.async_queries.ChartDataQueryContextSchema")
+def test_load_chart_data_into_cache_with_cache_load_error_does_not_reraise(
+    mock_query_context_schema_cls: mock.MagicMock,
+    mock_command_cls: mock.MagicMock,
+    mock_async_query_manager: mock.MagicMock,
+    mock_security_manager: mock.MagicMock,
+) -> None:
+    """Same as above, for the sibling 422-mapped ChartDataCacheLoadError."""
+    from superset.tasks.async_queries import load_chart_data_into_cache
+
+    job_metadata = {"user_id": 1}
+    form_data: dict[str, Any] = {}
+    err_message = "Cache load failed"
+
+    mock_security_manager.get_user_by_id.return_value = mock.MagicMock()
+    mock_async_query_manager.STATUS_ERROR = "error"
+    mock_query_context_schema_cls.return_value.load.return_value = mock.MagicMock()
+    mock_command_cls.return_value.run.side_effect = ChartDataCacheLoadError(err_message)
+
+    # Should not raise.
+    load_chart_data_into_cache(job_metadata, form_data)
+
+    mock_async_query_manager.update_job.assert_called_once_with(
+        job_metadata, "error", errors=[{"message": err_message}]
+    )
+
+
 @mock.patch("superset.tasks.async_queries.security_manager")
 @mock.patch("superset.tasks.async_queries.async_query_manager")
 @mock.patch("superset.tasks.async_queries.ChartDataQueryContextSchema")