feat: generic marshmallow error component (#25303)

superset-frontend/src/components/Datasource/DatasourceModal.test.jsx

@@ -96,7 +96,7 @@ describe('DatasourceModal', () => {
   it('saves on confirm', async () => {
     const callsP = fetchMock.post(SAVE_ENDPOINT, SAVE_PAYLOAD);
     fetchMock.put(SAVE_DATASOURCE_ENDPOINT, {});
-    fetchMock.get(GET_DATASOURCE_ENDPOINT, {});
+    fetchMock.get(GET_DATASOURCE_ENDPOINT, { result: {} });
     act(() => {
       wrapper
         .find('button[data-test="datasource-modal-save"]')

superset-frontend/src/components/Datasource/DatasourceModal.tsx

@@ -31,7 +31,11 @@ import {
 
 import Modal from 'src/components/Modal';
 import AsyncEsmComponent from 'src/components/AsyncEsmComponent';
-import { getClientErrorObject } from 'src/utils/getClientErrorObject';
+import {
+  genericSupersetError,
+  SupersetError,
+} from 'src/components/ErrorMessage/types';
+import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
 import withToasts from 'src/components/MessageToasts/withToasts';
 import { useSelector } from 'react-redux';
 
@@ -203,11 +207,18 @@ const DatasourceModal: FunctionComponent<DatasourceModalProps> = ({
       })
       .catch(response => {
         setIsSaving(false);
-        getClientErrorObject(response).then(({ error }) => {
+        response.json().then((errorJson: { errors: SupersetError[] }) => {
           modal.error({
-            title: t('Error'),
-            content: error || t('An error has occurred'),
+            title: t('Error saving dataset'),
             okButtonProps: { danger: true, className: 'btn-danger' },
+            content: (
+              <ErrorMessageWithStackTrace
+                error={
+                  errorJson?.errors?.[0] || genericSupersetError(errorJson)
+                }
+                source="crud"
+              />
+            ),
           });
         });
       });

superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.test.tsx

@@ -0,0 +1,86 @@
+/**
+ * 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 React from 'react';
+import { render, screen, fireEvent } from '@testing-library/react';
+import '@testing-library/jest-dom/extend-expect';
+import { ThemeProvider, supersetTheme } from '@superset-ui/core';
+import { ErrorLevel, ErrorTypeEnum } from 'src/components/ErrorMessage/types';
+import MarshmallowErrorMessage from './MarshmallowErrorMessage';
+
+describe('MarshmallowErrorMessage', () => {
+  const mockError = {
+    extra: {
+      messages: {
+        name: ["can't be blank"],
+        age: {
+          inner: ['is too low'],
+        },
+      },
+      payload: {
+        name: '',
+        age: {
+          inner: 10,
+        },
+      },
+      issue_codes: [],
+    },
+    message: 'Validation failed',
+    error_type: ErrorTypeEnum.MARSHMALLOW_ERROR,
+    level: 'error' as ErrorLevel,
+  };
+
+  test('renders without crashing', () => {
+    render(
+      <ThemeProvider theme={supersetTheme}>
+        <MarshmallowErrorMessage error={mockError} />
+      </ThemeProvider>,
+    );
+    expect(screen.getByText('Validation failed')).toBeInTheDocument();
+  });
+
+  test('renders the provided subtitle', () => {
+    render(
+      <ThemeProvider theme={supersetTheme}>
+        <MarshmallowErrorMessage error={mockError} subtitle="Error Alert" />
+      </ThemeProvider>,
+    );
+    expect(screen.getByText('Error Alert')).toBeInTheDocument();
+  });
+
+  test('renders extracted invalid values', () => {
+    render(
+      <ThemeProvider theme={supersetTheme}>
+        <MarshmallowErrorMessage error={mockError} />
+      </ThemeProvider>,
+    );
+    expect(screen.getByText("can't be blank:")).toBeInTheDocument();
+    expect(screen.getByText('is too low: 10')).toBeInTheDocument();
+  });
+
+  test('renders the JSONTree when details are expanded', () => {
+    render(
+      <ThemeProvider theme={supersetTheme}>
+        <MarshmallowErrorMessage error={mockError} />
+      </ThemeProvider>,
+    );
+    fireEvent.click(screen.getByText('Details'));
+    expect(screen.getByText('"can\'t be blank"')).toBeInTheDocument();
+  });
+});

superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.tsx

@@ -0,0 +1,109 @@
+/**
+ * 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 React from 'react';
+import { JSONTree } from 'react-json-tree';
+import { css, styled, SupersetTheme, t } from '@superset-ui/core';
+
+import { useJsonTreeTheme } from 'src/hooks/useJsonTreeTheme';
+import Collapse from 'src/components/Collapse';
+import { ErrorMessageComponentProps } from './types';
+
+interface MarshmallowErrorExtra {
+  messages: object;
+  payload: object;
+  issue_codes: {
+    code: number;
+    message: string;
+  }[];
+}
+
+const StyledUl = styled.ul`
+  padding-left: ${({ theme }) => theme.gridUnit * 5}px;
+  padding-top: ${({ theme }) => theme.gridUnit * 4}px;
+`;
+
+const collapseStyle = (theme: SupersetTheme) => css`
+  .ant-collapse-arrow {
+    left: 0px !important;
+  }
+  .ant-collapse-header {
+    padding-left: ${theme.gridUnit * 4}px !important;
+  }
+  .ant-collapse-content-box {
+    padding: 0px !important;
+  }
+`;
+
+const extractInvalidValues = (messages: object, payload: object): string[] => {
+  const invalidValues: string[] = [];
+
+  const recursiveExtract = (messages: object, payload: object) => {
+    Object.keys(messages).forEach(key => {
+      const value = payload[key];
+      const message = messages[key];
+
+      if (Array.isArray(message)) {
+        message.forEach(errorMessage => {
+          invalidValues.push(`${errorMessage}: ${value}`);
+        });
+      } else {
+        recursiveExtract(message, value);
+      }
+    });
+  };
+  recursiveExtract(messages, payload);
+  return invalidValues;
+};
+
+export default function MarshmallowErrorMessage({
+  error,
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  source = 'crud',
+  subtitle,
+}: ErrorMessageComponentProps<MarshmallowErrorExtra>) {
+  const jsonTreeTheme = useJsonTreeTheme();
+  const { extra, message } = error;
+
+  return (
+    <div>
+      {subtitle && <h4>{subtitle}</h4>}
+
+      {message}
+
+      <StyledUl>
+        {extractInvalidValues(extra.messages, extra.payload).map(
+          (value, index) => (
+            <li key={index}>{value}</li>
+          ),
+        )}
+      </StyledUl>
+
+      <Collapse ghost css={collapseStyle}>
+        <Collapse.Panel header={t('Details')} key="details" css={collapseStyle}>
+          <JSONTree
+            data={extra.messages}
+            shouldExpandNode={() => true}
+            hideRoot
+            theme={jsonTreeTheme}
+          />
+        </Collapse.Panel>
+      </Collapse>
+    </div>
+  );
+}

superset-frontend/src/components/ErrorMessage/types.ts

@@ -79,6 +79,7 @@ export const ErrorTypeEnum = {
   // API errors
   INVALID_PAYLOAD_FORMAT_ERROR: 'INVALID_PAYLOAD_FORMAT_ERROR',
   INVALID_PAYLOAD_SCHEMA_ERROR: 'INVALID_PAYLOAD_SCHEMA_ERROR',
+  MARSHMALLOW_ERROR: 'MARSHMALLOW_ERROR',
 } as const;
 
 type ValueOf<T> = T[keyof T];
@@ -88,7 +89,7 @@ export type ErrorType = ValueOf<typeof ErrorTypeEnum>;
 // Keep in sync with superset/views/errors.py
 export type ErrorLevel = 'info' | 'warning' | 'error';
 
-export type ErrorSource = 'dashboard' | 'explore' | 'sqllab';
+export type ErrorSource = 'dashboard' | 'explore' | 'sqllab' | 'crud';
 
 export type SupersetError<ExtraType = Record<string, any> | null> = {
   error_type: ErrorType;
@@ -106,3 +107,12 @@ export type ErrorMessageComponentProps<ExtraType = Record<string, any> | null> =
 
 export type ErrorMessageComponent =
   React.ComponentType<ErrorMessageComponentProps>;
+
+/* Generic error to be returned when the backend returns an error response that is not
+ * SIP-41 compliant. */
+export const genericSupersetError = (extra: object) => ({
+  error_type: ErrorTypeEnum.GENERIC_BACKEND_ERROR,
+  extra,
+  level: 'error' as ErrorLevel,
+  message: 'An error occurred',
+});

superset-frontend/src/components/FilterableTable/index.tsx

@@ -24,9 +24,9 @@ import {
   t,
   safeHtmlSpan,
   styled,
-  useTheme,
 } from '@superset-ui/core';
 import { useDebounceValue } from 'src/hooks/useDebounceValue';
+import { useJsonTreeTheme } from 'src/hooks/useJsonTreeTheme';
 import Button from '../Button';
 import CopyToClipboard from '../CopyToClipboard';
 import ModalTrigger from '../ModalTrigger';
@@ -183,32 +183,7 @@ const FilterableTable = ({
     return complexColumns[columnKey] ? truncated : content;
   };
 
-  const theme = useTheme();
-  const [jsonTreeTheme, setJsonTreeTheme] = useState<Record<string, string>>();
-
-  const getJsonTreeTheme = () => {
-    if (!jsonTreeTheme) {
-      setJsonTreeTheme({
-        base00: theme.colors.grayscale.dark2,
-        base01: theme.colors.grayscale.dark1,
-        base02: theme.colors.grayscale.base,
-        base03: theme.colors.grayscale.light1,
-        base04: theme.colors.grayscale.light2,
-        base05: theme.colors.grayscale.light3,
-        base06: theme.colors.grayscale.light4,
-        base07: theme.colors.grayscale.light5,
-        base08: theme.colors.error.base,
-        base09: theme.colors.error.light1,
-        base0A: theme.colors.error.light2,
-        base0B: theme.colors.success.base,
-        base0C: theme.colors.primary.light1,
-        base0D: theme.colors.primary.base,
-        base0E: theme.colors.primary.dark1,
-        base0F: theme.colors.error.dark1,
-      });
-    }
-    return jsonTreeTheme;
-  };
+  const jsonTreeTheme = useJsonTreeTheme();
 
   const getWidthsForColumns = () => {
     const PADDING = 50; // accounts for cell padding and width of sorting icon
@@ -293,7 +268,7 @@ const FilterableTable = ({
       modalBody={
         <JSONTree
           data={jsonObject}
-          theme={getJsonTreeTheme()}
+          theme={jsonTreeTheme}
           valueRenderer={renderBigIntStrToNumber}
         />
       }

superset-frontend/src/hooks/useJsonTreeTheme.ts

@@ -0,0 +1,41 @@
+/**
+ * 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 { useTheme } from '@superset-ui/core';
+
+export const useJsonTreeTheme = () => {
+  const theme = useTheme();
+  return {
+    base00: theme.colors.grayscale.dark2,
+    base01: theme.colors.grayscale.dark1,
+    base02: theme.colors.grayscale.base,
+    base03: theme.colors.grayscale.light1,
+    base04: theme.colors.grayscale.light2,
+    base05: theme.colors.grayscale.light3,
+    base06: theme.colors.grayscale.light4,
+    base07: theme.colors.grayscale.light5,
+    base08: theme.colors.error.base,
+    base09: theme.colors.error.light1,
+    base0A: theme.colors.error.light2,
+    base0B: theme.colors.success.base,
+    base0C: theme.colors.primary.light1,
+    base0D: theme.colors.primary.base,
+    base0E: theme.colors.primary.dark1,
+    base0F: theme.colors.error.dark1,
+  };
+};

superset-frontend/src/setup/setupErrorMessages.ts

@@ -20,6 +20,7 @@ import getErrorMessageComponentRegistry from 'src/components/ErrorMessage/getErr
 import { ErrorTypeEnum } from 'src/components/ErrorMessage/types';
 import TimeoutErrorMessage from 'src/components/ErrorMessage/TimeoutErrorMessage';
 import DatabaseErrorMessage from 'src/components/ErrorMessage/DatabaseErrorMessage';
+import MarshmallowErrorMessage from 'src/components/ErrorMessage/MarshmallowErrorMessage';
 import ParameterErrorMessage from 'src/components/ErrorMessage/ParameterErrorMessage';
 import DatasetNotFoundErrorMessage from 'src/components/ErrorMessage/DatasetNotFoundErrorMessage';
 
@@ -144,5 +145,9 @@ export default function setupErrorMessages() {
     ErrorTypeEnum.FAILED_FETCHING_DATASOURCE_INFO_ERROR,
     DatasetNotFoundErrorMessage,
   );
+  errorMessageComponentRegistry.registerValue(
+    ErrorTypeEnum.MARSHMALLOW_ERROR,
+    MarshmallowErrorMessage,
+  );
   setupErrorMessagesExtra();
 }

superset/datasets/api.py

@@ -332,7 +332,6 @@ class DatasetRestApi(BaseSupersetModelRestApi):
 
     @expose("/<pk>", methods=("PUT",))
     @protect()
-    @safe
     @statsd_metrics
     @event_logger.log_this_with_context(
         action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put",

superset/datasets/schemas.py

@@ -24,6 +24,7 @@ from marshmallow.validate import Length
 from marshmallow_sqlalchemy import SQLAlchemyAutoSchema
 
 from superset.datasets.models import Dataset
+from superset.exceptions import SupersetMarshmallowValidationError
 
 get_delete_ids_schema = {"type": "array", "items": {"type": "integer"}}
 get_export_ids_schema = {"type": "array", "items": {"type": "integer"}}
@@ -125,6 +126,17 @@ class DatasetPutSchema(Schema):
     is_managed_externally = fields.Boolean(allow_none=True, dump_default=False)
     external_url = fields.String(allow_none=True)
 
+    def handle_error(
+        self,
+        error: ValidationError,
+        data: dict[str, Any],
+        **kwargs: Any,
+    ) -> None:
+        """
+        Return SIP-40 error.
+        """
+        raise SupersetMarshmallowValidationError(error, data)
+
 
 class DatasetDuplicateSchema(Schema):
     base_model_id = fields.Integer(required=True)

superset/errors.py

@@ -90,6 +90,7 @@ class SupersetErrorType(StrEnum):
     # API errors
     INVALID_PAYLOAD_FORMAT_ERROR = "INVALID_PAYLOAD_FORMAT_ERROR"
     INVALID_PAYLOAD_SCHEMA_ERROR = "INVALID_PAYLOAD_SCHEMA_ERROR"
+    MARSHMALLOW_ERROR = "MARSHMALLOW_ERROR"
 
     # Report errors
     REPORT_NOTIFICATION_ERROR = "REPORT_NOTIFICATION_ERROR"
@@ -144,6 +145,7 @@ ISSUE_CODES = {
     1035: _("Failed to start remote query on a worker."),
     1036: _("The database was deleted."),
     1037: _("Custom SQL fields cannot contain sub-queries."),
+    1040: _("The submitted payload failed validation."),
 }
 
 
@@ -181,6 +183,7 @@ ERROR_TYPES_TO_ISSUE_CODES_MAPPING = {
     SupersetErrorType.ASYNC_WORKERS_ERROR: [1035],
     SupersetErrorType.DATABASE_NOT_FOUND_ERROR: [1011, 1036],
     SupersetErrorType.CONNECTION_DATABASE_TIMEOUT: [1001, 1009],
+    SupersetErrorType.MARSHMALLOW_ERROR: [1040],
 }
 
 

superset/exceptions.py

@@ -278,3 +278,20 @@ class QueryNotFoundException(SupersetException):
 
 class ColumnNotFoundException(SupersetException):
     status = 404
+
+
+class SupersetMarshmallowValidationError(SupersetErrorException):
+    """
+    Exception to be raised for Marshmallow validation errors.
+    """
+
+    status = 422
+
+    def __init__(self, exc: ValidationError, payload: dict[str, Any]):
+        error = SupersetError(
+            message=_("The schema of the submitted payload is invalid."),
+            error_type=SupersetErrorType.MARSHMALLOW_ERROR,
+            level=ErrorLevel.ERROR,
+            extra={"messages": exc.messages, "payload": payload},
+        )
+        super().__init__(error)

tests/unit_tests/datasets/api_tests.py

@@ -0,0 +1,73 @@
+# 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.
+
+from typing import Any
+
+from sqlalchemy.orm.session import Session
+
+
+def test_put_invalid_dataset(
+    session: Session,
+    client: Any,
+    full_api_access: None,
+) -> None:
+    """
+    Test invalid payloads.
+    """
+    from superset.connectors.sqla.models import SqlaTable
+    from superset.models.core import Database
+
+    SqlaTable.metadata.create_all(session.get_bind())
+
+    database = Database(
+        database_name="my_db",
+        sqlalchemy_uri="sqlite://",
+    )
+    dataset = SqlaTable(
+        table_name="test_put_invalid_dataset",
+        database=database,
+    )
+    session.add(dataset)
+    session.flush()
+
+    response = client.put(
+        "/api/v1/dataset/1",
+        json={"invalid": "payload"},
+    )
+    assert response.status_code == 422
+    assert response.json == {
+        "errors": [
+            {
+                "message": "The schema of the submitted payload is invalid.",
+                "error_type": "MARSHMALLOW_ERROR",
+                "level": "error",
+                "extra": {
+                    "messages": {"invalid": ["Unknown field."]},
+                    "payload": {"invalid": "payload"},
+                    "issue_codes": [
+                        {
+                            "code": 1040,
+                            "message": (
+                                "Issue 1040 - The submitted payload "
+                                "failed validation."
+                            ),
+                        }
+                    ],
+                },
+            }
+        ]
+    }