fix(dashboard): stretch Chart Data modal results grid to fill available height

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.22",
+        "dayjs": "^1.11.21",
         "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.5",
+        "concurrently": "^10.0.4",
         "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.5",
-      "resolved": "https://registry.npmjs.org/concurrently/-/concurrently-10.0.5.tgz",
-      "integrity": "sha512-JaP/CoftUrCcAFW/g//RbgEGwlelnEae6cfBLgH6ZdO6s8jPkn6p9SB9u6pdVxYXoiSnFqseOlHfrEfF82TVOg==",
+      "version": "10.0.4",
+      "resolved": "https://registry.npmjs.org/concurrently/-/concurrently-10.0.4.tgz",
+      "integrity": "sha512-trZql+7l/0+WRAsAnEdctr4+iiOS6ZrViI6H8QWcCF9MFS/LT0dKpe8vluB1to6it+OxSI4VospFTIFMW8DJRw==",
       "dev": true,
       "license": "MIT",
       "dependencies": {
@@ -18541,9 +18541,9 @@
       }
     },
     "node_modules/dayjs": {
-      "version": "1.11.22",
-      "resolved": "https://registry.npmjs.org/dayjs/-/dayjs-1.11.22.tgz",
-      "integrity": "sha512-1YRnxzt/AabP3GHxnaB9/b+ZScCKu5TeF+co+BWG+lnWVIwEcTFc1FVE0WLNmNO3sA6GGXL40i5qkHfbLzpwrg==",
+      "version": "1.11.21",
+      "resolved": "https://registry.npmjs.org/dayjs/-/dayjs-1.11.21.tgz",
+      "integrity": "sha512-98IT+HOahAisibz/yjKbzuOBwYcjJ7BCLPzARyHiyEBmRz4fatF+KPJszEHXsGYjUG234aH/cOjW1wwTbKUZlA==",
       "license": "MIT"
     },
     "node_modules/debounce": {
@@ -34873,18 +34873,12 @@
       }
     },
     "node_modules/react-error-boundary": {
-      "version": "6.1.3",
-      "resolved": "https://registry.npmjs.org/react-error-boundary/-/react-error-boundary-6.1.3.tgz",
-      "integrity": "sha512-GnSKpCohFi2nQmJCWwP8O8wub7zexlePvpsejvQr35vS5RTouS1+utTNOmyc540yw5vyOXnSL1rBWsCQDmkyUA==",
+      "version": "6.1.2",
+      "resolved": "https://registry.npmjs.org/react-error-boundary/-/react-error-boundary-6.1.2.tgz",
+      "integrity": "sha512-3DpCr5HVdZ0caUjYE/kIHBEJN0mNP3ZCgf16c48uJ5TbWjorKVp+YG8W3XqlJ7vJAVNw6wNIImyPXmFydwmyng==",
       "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": {
@@ -42968,7 +42962,7 @@
         "d3-scale": "^4.0.2",
         "d3-time": "^3.1.0",
         "d3-time-format": "^4.1.0",
-        "dayjs": "^1.11.22",
+        "dayjs": "^1.11.21",
         "dompurify": "^3.4.13",
         "fetch-retry": "^6.0.0",
         "handlebars": "^4.7.9",
@@ -42980,7 +42974,7 @@
         "re-resizable": "^6.11.2",
         "react-ace": "^14.0.1",
         "react-draggable": "^4.7.1",
-        "react-error-boundary": "^6.1.3",
+        "react-error-boundary": "^6.1.2",
         "react-js-cron": "^6.0.2",
         "react-markdown": "^10.1.0",
         "react-resize-detector": "^7.1.2",
@@ -43079,6 +43073,15 @@
         "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.22",
+    "dayjs": "^1.11.21",
     "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.5",
+    "concurrently": "^10.0.4",
     "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.22",
+    "dayjs": "^1.11.21",
     "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.3",
+    "react-error-boundary": "^6.1.2",
     "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,6 +779,35 @@ 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;
@@ -2140,7 +2169,7 @@ function DatasourceEditor({
           }}
           expandFieldset={
             <FormContainer>
-              <Fieldset compact>
+              <Fieldset compact renderWarning={renderMetricFormatWarning}>
                 <Field
                   fieldKey="expression"
                   label={t('SQL expression')}

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

@@ -0,0 +1,91 @@
+/**
+ * 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,6 +28,7 @@ export interface FieldsetProps {
   item?: Record<string, any>;
   title?: ReactNode;
   compact?: boolean;
+  renderWarning?: (item: Record<string, any>) => ReactNode;
 }
 
 type fieldKeyType = string | number;
@@ -38,6 +39,7 @@ 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
@@ -78,6 +80,7 @@ export default function Fieldset({
         </Typography.Title>
       )}
 
+      {renderWarning?.(item)}
       {recurseReactClone(children, Field, propExtender)}
     </Form>
   );

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

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

superset/tasks/async_queries.py

@@ -106,10 +106,6 @@ 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:
@@ -127,20 +123,6 @@ 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,11 +110,8 @@ class TestAsyncQueries(SupersetTestCase):
             "status": "pending",
             "errors": [],
         }
-        # 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)
+        with pytest.raises(ChartDataQueryFailedError):
+            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,10 +21,7 @@ import pytest
 from celery.exceptions import SoftTimeLimitExceeded
 from flask_babel import lazy_gettext as _
 
-from superset.commands.chart.exceptions import (
-    ChartDataCacheLoadError,
-    ChartDataQueryFailedError,
-)
+from superset.commands.chart.exceptions import ChartDataQueryFailedError
 from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
 from superset.exceptions import (
     OAuth2RedirectError,
@@ -46,7 +43,7 @@ def test_load_chart_data_into_cache_with_error(
     job_metadata = {"user_id": 1}
     form_data = {}
     err_message = "Something went wrong"
-    err = RuntimeError(err_message)
+    err = ChartDataQueryFailedError(_(err_message))
 
     mock_user = mock.MagicMock()
     mock_query_context_schema = mock.MagicMock()
@@ -57,7 +54,7 @@ def test_load_chart_data_into_cache_with_error(
 
     mock_query_context_schema.load.side_effect = err
 
-    with pytest.raises(RuntimeError):
+    with pytest.raises(ChartDataQueryFailedError):
         load_chart_data_into_cache(job_metadata, form_data)
 
     expected_errors = [{"message": err_message}]
@@ -67,76 +64,6 @@ 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")