chore(native-filters): Ensure consistent error handling (apache#24206)

Co-authored-by: Michael S. Molina <michael.s.molina@gmail.com> (cherry picked from commit 674da1b)
airbnb · May 30, 2023 · 5235181 · 5235181
1 parent 02d3d53
commit 5235181
Show file tree

Hide file tree

Showing 5 changed files with 75 additions and 31 deletions.
diff --git a/superset-frontend/src/components/ErrorMessage/BasicErrorAlert.tsx b/superset-frontend/src/components/ErrorMessage/BasicErrorAlert.tsx
@@ -37,6 +37,7 @@ const StyledContent = styled.div`
   display: flex;
   flex-direction: column;
   margin-left: ${({ theme }) => theme.gridUnit * 2}px;
+  overflow: hidden;
 `;
 
 const StyledTitle = styled.span`

diff --git a/superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.test.tsx b/superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.test.tsx
@@ -21,7 +21,8 @@ import React from 'react';
 import { render, screen } from 'spec/helpers/testing-library';
 import userEvent from '@testing-library/user-event';
 import ErrorMessageWithStackTrace from './ErrorMessageWithStackTrace';
-import { ErrorLevel, ErrorSource } from './types';
+import BasicErrorAlert from './BasicErrorAlert';
+import { ErrorLevel, ErrorSource, ErrorTypeEnum } from './types';
 
 jest.mock(
   'src/components/Icons/Icon',
@@ -57,3 +58,21 @@ test('should render the link', () => {
   expect(link).toHaveTextContent('(Request Access)');
   expect(link).toHaveAttribute('href', mockedProps.link);
 });
+
+test('should render the fallback', () => {
+  const body = 'Blahblah';
+  render(
+    <ErrorMessageWithStackTrace
+      error={{
+        error_type: ErrorTypeEnum.FRONTEND_NETWORK_ERROR,
+        message: body,
+        extra: {},
+        level: 'error',
+      }}
+      fallback={<BasicErrorAlert title="Blah" body={body} level="error" />}
+      {...mockedProps}
+    />,
+    { useRedux: true },
+  );
+  expect(screen.getByText(body)).toBeInTheDocument();
+});
diff --git a/superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.tsx b/superset-frontend/src/components/ErrorMessage/ErrorMessageWithStackTrace.tsx
@@ -34,6 +34,7 @@ type Props = {
   source?: ErrorSource;
   description?: string;
   errorMitigationFunction?: () => void;
+  fallback?: React.ReactNode;
 };
 
 export default function ErrorMessageWithStackTrace({
@@ -45,6 +46,7 @@ export default function ErrorMessageWithStackTrace({
   stackTrace,
   source,
   description,
+  fallback,
 }: Props) {
   // Check if a custom error message component was registered for this message
   if (error) {
@@ -62,6 +64,10 @@ export default function ErrorMessageWithStackTrace({
     }
   }
 
+  if (fallback) {
+    return <>{fallback}</>;
+  }
+
   return (
     <ErrorAlert
       level="warning"

diff --git a/...-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx b/...-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
@@ -40,9 +40,13 @@ import { isEqual, isEqualWith } from 'lodash';
 import { getChartDataRequest } from 'src/components/Chart/chartAction';
 import Loading from 'src/components/Loading';
 import BasicErrorAlert from 'src/components/ErrorMessage/BasicErrorAlert';
+import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
 import { isFeatureEnabled } from 'src/featureFlags';
 import { waitForAsyncData } from 'src/middleware/asyncEvent';
-import { ClientErrorObject } from 'src/utils/getClientErrorObject';
+import {
+  ClientErrorObject,
+  getClientErrorObject,
+} from 'src/utils/getClientErrorObject';
 import { FilterBarOrientation, RootState } from 'src/dashboard/types';
 import {
   onFiltersRefreshSuccess,
@@ -97,7 +101,7 @@ const FilterValue: React.FC<FilterControlProps> = ({
   const dependencies = useFilterDependencies(id, dataMaskSelected);
   const shouldRefresh = useShouldFilterRefresh();
   const [state, setState] = useState<ChartDataResponseResult[]>([]);
-  const [error, setError] = useState<string>('');
+  const [error, setError] = useState<ClientErrorObject>();
   const [formData, setFormData] = useState<Partial<QueryFormData>>({
     inView: false,
   });
@@ -183,11 +187,11 @@ const FilterValue: React.FC<FilterControlProps> = ({
                   setState(asyncResult);
                   handleFilterLoadFinish();
                 })
-                .catch((error: ClientErrorObject) => {
-                  setError(
-                    error.message || error.error || t('Check configuration'),
-                  );
-                  handleFilterLoadFinish();
+                .catch((error: Response) => {
+                  getClientErrorObject(error).then(clientErrorObject => {
+                    setError(clientErrorObject);
+                    handleFilterLoadFinish();
+                  });
                 });
             } else {
               throw new Error(
@@ -196,13 +200,15 @@ const FilterValue: React.FC<FilterControlProps> = ({
             }
           } else {
             setState(json.result);
-            setError('');
+            setError(undefined);
             handleFilterLoadFinish();
           }
         })
         .catch((error: Response) => {
-          setError(error.statusText);
-          handleFilterLoadFinish();
+          getClientErrorObject(error).then(clientErrorObject => {
+            setError(clientErrorObject);
+            handleFilterLoadFinish();
+          });
         });
     }
   }, [
@@ -298,10 +304,15 @@ const FilterValue: React.FC<FilterControlProps> = ({
 
   if (error) {
     return (
-      <BasicErrorAlert
-        title={t('Cannot load filter')}
-        body={error}
-        level="error"
+      <ErrorMessageWithStackTrace
+        error={error.errors?.[0]}
+        fallback={
+          <BasicErrorAlert
+            title={t('Cannot load filter')}
+            body={error.error}
+            level="error"
+          />
+        }
       />
     );
   }

diff --git a/...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx
@@ -54,6 +54,7 @@ import { Input, TextArea } from 'src/components/Input';
 import { Select, FormInstance } from 'src/components';
 import Collapse from 'src/components/Collapse';
 import BasicErrorAlert from 'src/components/ErrorMessage/BasicErrorAlert';
+import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
 import { FormItem } from 'src/components/Form';
 import Icons from 'src/components/Icons';
 import Loading from 'src/components/Loading';
@@ -72,7 +73,10 @@ import DateFilterControl from 'src/explore/components/controls/DateFilterControl
 import AdhocFilterControl from 'src/explore/components/controls/FilterControl/AdhocFilterControl';
 import { isFeatureEnabled } from 'src/featureFlags';
 import { waitForAsyncData } from 'src/middleware/asyncEvent';
-import { ClientErrorObject } from 'src/utils/getClientErrorObject';
+import {
+  ClientErrorObject,
+  getClientErrorObject,
+} from 'src/utils/getClientErrorObject';
 import { SingleValueType } from 'src/filters/components/Range/SingleValueType';
 import {
   getFormData,
@@ -345,7 +349,7 @@ const FiltersConfigForm = (
   ref: React.RefObject<any>,
 ) => {
   const isRemoved = !!removedFilters[filterId];
-  const [error, setError] = useState<string>('');
+  const [error, setError] = useState<ClientErrorObject>();
   const [metrics, setMetrics] = useState<Metric[]>([]);
   const [activeTabKey, setActiveTabKey] = useState<string>(
     FilterTabs.configuration.key,
@@ -440,11 +444,11 @@ const FiltersConfigForm = (
 
   const setNativeFilterFieldValuesWrapper = (values: object) => {
     setNativeFilterFieldValues(form, filterId, values);
-    setError('');
+    setError(undefined);
     forceUpdate();
   };
 
-  const setErrorWrapper = (error: string) => {
+  const setErrorWrapper = (error: ClientErrorObject) => {
     setNativeFilterFieldValues(form, filterId, {
       defaultValueQueriesData: null,
     });
@@ -506,10 +510,10 @@ const FiltersConfigForm = (
                     defaultValueQueriesData: asyncResult,
                   });
                 })
-                .catch((error: ClientErrorObject) => {
-                  setError(
-                    error.message || error.error || t('Check configuration'),
-                  );
+                .catch((error: Response) => {
+                  getClientErrorObject(error).then(clientErrorObject => {
+                    setErrorWrapper(clientErrorObject);
+                  });
                 });
             } else {
               throw new Error(
@@ -523,10 +527,8 @@ const FiltersConfigForm = (
           }
         })
         .catch((error: Response) => {
-          error.json().then(body => {
-            setErrorWrapper(
-              body.message || error.statusText || t('Check configuration'),
-            );
+          getClientErrorObject(error).then(clientErrorObject => {
+            setError(clientErrorObject);
           });
         });
     },
@@ -1224,10 +1226,15 @@ const FiltersConfigForm = (
                     {error || showDefaultValue ? (
                       <DefaultValueContainer>
                         {error ? (
-                          <BasicErrorAlert
-                            title={t('Cannot load filter')}
-                            body={error}
-                            level="error"
+                          <ErrorMessageWithStackTrace
+                            error={error.errors?.[0]}
+                            fallback={
+                              <BasicErrorAlert
+                                title={t('Cannot load filter')}
+                                body={error.error}
+                                level="error"
+                              />
+                            }
                           />
                         ) : (
                           <DefaultValue