fix(charts): improve error display for failed charts in dashboards (#37939)

superset-frontend/src/components/Chart/Chart.test.tsx

@@ -0,0 +1,88 @@
+/**
+ * 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 { render, screen } from 'spec/helpers/testing-library';
+import '@testing-library/jest-dom';
+import { PLACEHOLDER_DATASOURCE } from 'src/dashboard/constants';
+import { ResourceStatus } from 'src/hooks/apiResources/apiResources';
+import Chart from './Chart';
+import type { Actions } from './Chart';
+
+const mockActions: Actions = {
+  logEvent: jest.fn() as unknown as Actions['logEvent'],
+  chartRenderingFailed: jest.fn() as unknown as Actions['chartRenderingFailed'],
+  chartRenderingSucceeded:
+    jest.fn() as unknown as Actions['chartRenderingSucceeded'],
+  postChartFormData: jest.fn() as unknown as Actions['postChartFormData'],
+};
+
+const baseProps = {
+  chartId: 1,
+  width: 800,
+  height: 600,
+  actions: mockActions,
+  formData: { datasource: '1__table', viz_type: 'table' },
+  vizType: 'table',
+  setControlValue: jest.fn(),
+};
+
+test('shows backend error instead of loading spinner when datasource is still a placeholder', () => {
+  render(
+    <Chart
+      {...baseProps}
+      chartStatus="failed"
+      chartAlert="Your default credentials were not found."
+      datasource={PLACEHOLDER_DATASOURCE}
+      datasetsStatus={ResourceStatus.Loading}
+      queriesResponse={[
+        {
+          errors: [
+            {
+              error_type: 'GENERIC_BACKEND_ERROR',
+              message: 'Your default credentials were not found.',
+              extra: {
+                issue_codes: [{ code: 1011, message: 'Issue 1011' }],
+              },
+              level: 'error',
+            },
+          ],
+        },
+      ]}
+    />,
+  );
+
+  expect(
+    screen.getByText(/Your default credentials were not found/),
+  ).toBeInTheDocument();
+});
+
+test('shows loading spinner for client-side errors without errors array when datasource is still a placeholder', () => {
+  render(
+    <Chart
+      {...baseProps}
+      chartStatus="failed"
+      chartAlert="Some client-side error"
+      datasource={PLACEHOLDER_DATASOURCE}
+      datasetsStatus={ResourceStatus.Loading}
+      queriesResponse={[{}]}
+    />,
+  );
+
+  expect(screen.getByRole('status')).toBeInTheDocument();
+  expect(screen.queryByText(/Some client-side error/)).not.toBeInTheDocument();
+});

superset-frontend/src/components/Chart/Chart.tsx

@@ -168,6 +168,11 @@ const LoadingDiv = styled.div`
   transform: translate(-50%, -50%);
 `;
 
+const ErrorContainer = styled.div<{ height: number }>`
+  height: ${p => p.height}px;
+  overflow: auto;
+`;
+
 const MessageSpan = styled.span`
   display: block;
   text-align: center;
@@ -261,7 +266,10 @@ class Chart extends PureComponent<ChartProps, {}> {
     const message = chartAlert || queryResponse?.message;
 
     // if datasource is still loading, don't render JS errors
+    // but always show backend API errors (which have an errors array)
+    // so users can see real issues like auth failures
     if (
+      !error &&
       chartAlert !== undefined &&
       chartAlert !== NONEXISTENT_DATASET &&
       datasource === PLACEHOLDER_DATASOURCE &&
@@ -353,8 +361,12 @@ class Chart extends PureComponent<ChartProps, {}> {
     const isLoading = chartStatus === 'loading';
 
     if (chartStatus === 'failed') {
-      return queriesResponse?.map(item =>
-        this.renderErrorMessage(item as ChartErrorType),
+      return (
+        <ErrorContainer height={height}>
+          {queriesResponse?.map(item =>
+            this.renderErrorMessage(item as ChartErrorType),
+          )}
+        </ErrorContainer>
       );
     }
 

superset/db_engine_specs/gsheets.py

@@ -203,13 +203,18 @@ class GSheetsEngineSpec(ShillelaghEngineSpec):
         In case the token was manually revoked on Google side, `google-auth` will
         try to automatically refresh credentials, but it fails since it only has the
         access token. This override catches this scenario as well.
+
+        Also catches the case where no credentials are configured at all
+        (missing Application Default Credentials).
         """
+        error_message = str(ex).lower()
         return (
             g
             and hasattr(g, "user")
             and (
                 isinstance(ex, cls.oauth2_exception)
-                or "credentials do not contain the necessary fields" in str(ex)
+                or "credentials do not contain the necessary fields" in error_message
+                or "default credentials were not found" in error_message
             )
         )
 

tests/unit_tests/db_engine_specs/test_gsheets.py

@@ -757,6 +757,26 @@ def test_needs_oauth2_with_credentials_error(mocker: MockerFixture) -> None:
     assert GSheetsEngineSpec.needs_oauth2(ex) is True
 
 
+def test_needs_oauth2_with_default_credentials_not_found(
+    mocker: MockerFixture,
+) -> None:
+    """
+    Test that needs_oauth2 returns True when Application Default Credentials
+    are not configured.
+    """
+    from superset.db_engine_specs.gsheets import GSheetsEngineSpec
+
+    g = mocker.patch("superset.db_engine_specs.gsheets.g")
+    g.user = mocker.MagicMock()
+
+    ex = Exception(
+        "Your default credentials were not found. To set up Application Default "
+        "Credentials, see https://cloud.google.com/docs/authentication/external/"
+        "set-up-adc for more information."
+    )
+    assert GSheetsEngineSpec.needs_oauth2(ex) is True
+
+
 def test_needs_oauth2_with_other_error(mocker: MockerFixture) -> None:
     """
     Test that needs_oauth2 returns False for other errors.