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/explore/components/DataTablesPane/DataTablesPane.tsx

@@ -29,7 +29,22 @@ import {
 } from 'src/utils/localStorageHelpers';
 import { SamplesPane, useResultsPane } from './components';
 import { DataTablesPaneProps, ResultTypes } from './types';
-import { getStaleResultsTabFallback } from './utils';
+
+/**
+ * A mixed chart can be reconfigured to return fewer result panes than before
+ * (e.g. dropping a query), which removes the corresponding results tab. If the
+ * selected tab was one of those, the active key goes stale and the data panel
+ * renders blank until the user reselects a valid tab. Returns the first
+ * results tab to fall back to in that case, otherwise undefined.
+ */
+export const getStaleResultsTabFallback = (
+  activeTabKey: string,
+  resultsTabKeys: string[],
+): string | undefined =>
+  activeTabKey.startsWith(ResultTypes.Results) &&
+  !resultsTabKeys.includes(activeTabKey)
+    ? ResultTypes.Results
+    : undefined;
 
 const StyledDiv = styled.div`
   ${() => `

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

@@ -20,15 +20,22 @@ import { t } from '@apache-superset/core/translation';
 import { styled } from '@apache-superset/core/theme';
 import Tabs from '@superset-ui/core/components/Tabs';
 import { ResultTypes, ResultsPaneProps } from '../types';
-import { getStaleResultsTabFallback } from '../utils';
 import { useResultsPane } from './useResultsPane';
-import { useEffect, useState } from 'react';
+import { useState } from 'react';
 
 const Wrapper = styled.div`
   display: flex;
   flex-direction: column;
   height: 100%;
 
+  .ant-tabs {
+    height: 100%;
+  }
+
+  .ant-tabs-body {
+    height: 100%;
+  }
+
   .ant-tabs-content {
     display: flex;
     flex-direction: column;
@@ -79,25 +86,9 @@ export const ResultsPaneOnDashboard = ({
     };
   });
 
-  const resultsTabFallback = getStaleResultsTabFallback(
-    activeTabKey,
-    items.map(({ key }) => key),
-  );
-
-  useEffect(() => {
-    if (resultsTabFallback) {
-      setActiveTabKey(resultsTabFallback);
-    }
-  }, [resultsTabFallback]);
-
   return (
     <Wrapper>
-      <Tabs
-        fullHeight
-        activeKey={activeTabKey}
-        onChange={setActiveTabKey}
-        items={items}
-      />
+      <Tabs activeKey={activeTabKey} onChange={setActiveTabKey} items={items} />
     </Wrapper>
   );
 };

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

@@ -20,34 +20,14 @@ import fetchMock from 'fetch-mock';
 import {
   screen,
   render,
-  act,
   waitForElementToBeRemoved,
   waitFor,
 } from 'spec/helpers/testing-library';
 import { ChartMetadata, ChartPlugin, VizType } from '@superset-ui/core';
 import { setupAGGridModules } from '@superset-ui/core/components/ThemedAgGridReact';
-import Tabs from '@superset-ui/core/components/Tabs';
 import { ResultsPaneOnDashboard } from '../components';
-import { useResultsPane } from '../components/useResultsPane';
 import { createResultsPaneOnDashboardProps } from './fixture';
 
-// `fullHeight`'s CSS isn't testable under jsdom (no `importSource` for the
-// `css` prop in jest's babel config), so spy on call args instead.
-jest.mock('@superset-ui/core/components/Tabs', () => {
-  const actual = jest.requireActual('@superset-ui/core/components/Tabs');
-  return { __esModule: true, ...actual, default: jest.fn(actual.default) };
-});
-
-// Wraps the real hook; only overridden below to avoid mounting a second
-// real AG Grid instance, which jsdom doesn't support.
-jest.mock('../components/useResultsPane', () => {
-  const actual = jest.requireActual('../components/useResultsPane');
-  return {
-    __esModule: true,
-    useResultsPane: jest.fn(actual.useResultsPane),
-  };
-});
-
 beforeAll(() => {
   setupAGGridModules();
 });
@@ -126,10 +106,6 @@ describe('ResultsPaneOnDashboard', () => {
     expect(
       await findByText('No results were returned for this query'),
     ).toBeVisible();
-    expect(Tabs).toHaveBeenCalledWith(
-      expect.objectContaining({ fullHeight: true }),
-      expect.anything(),
-    );
   });
 
   test('render errorMessage', async () => {
@@ -243,36 +219,4 @@ describe('ResultsPaneOnDashboard', () => {
     expect(tab2).toBeVisible();
     expect(tab3).toBeNull();
   });
-
-  test('falls back to the first results tab when the active one disappears', async () => {
-    const mockedUseResultsPane = useResultsPane as jest.Mock;
-    mockedUseResultsPane.mockReturnValue([<div key="a" />, <div key="b" />]);
-
-    const props = createResultsPaneOnDashboardProps({ sliceId: 999 });
-    const { rerender } = render(<ResultsPaneOnDashboard {...props} />, {
-      useRedux: true,
-    });
-
-    const latestTabsProps = () => {
-      const { calls } = (Tabs as unknown as jest.Mock).mock;
-      return calls[calls.length - 1][0];
-    };
-    expect(latestTabsProps().items.map((i: { key: string }) => i.key)).toEqual([
-      'results',
-      'results 2',
-    ]);
-
-    act(() => {
-      latestTabsProps().onChange('results 2');
-    });
-    expect(latestTabsProps().activeKey).toBe('results 2');
-
-    // A mixed chart dropped from two query results to one, removing "results 2"
-    mockedUseResultsPane.mockReturnValue([<div key="a" />]);
-    rerender(<ResultsPaneOnDashboard {...props} />);
-
-    await waitFor(() => {
-      expect(latestTabsProps().activeKey).toBe('results');
-    });
-  });
 });

superset-frontend/src/explore/components/DataTablesPane/test/getStaleResultsTabFallback.test.ts

@@ -16,7 +16,7 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { getStaleResultsTabFallback } from '../utils';
+import { getStaleResultsTabFallback } from '../DataTablesPane';
 import { ResultTypes } from '../types';
 
 test('keeps the active tab when it still exists', () => {

superset-frontend/src/explore/components/DataTablesPane/utils.ts

@@ -1,35 +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 { ResultTypes } from './types';
-
-/**
- * A mixed chart can be reconfigured to return fewer result panes than before
- * (e.g. dropping a query), which removes the corresponding results tab. If the
- * selected tab was one of those, the active key goes stale and the data panel
- * renders blank until the user reselects a valid tab. Returns the first
- * results tab to fall back to in that case, otherwise undefined.
- */
-export const getStaleResultsTabFallback = (
-  activeTabKey: string,
-  resultsTabKeys: string[],
-): string | undefined =>
-  activeTabKey.startsWith(ResultTypes.Results) &&
-  !resultsTabKeys.includes(activeTabKey)
-    ? ResultTypes.Results
-    : undefined;

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")