From 2445f3eda45602e5bed2301271b0b7897b02caca Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Tue, 3 Oct 2023 11:35:28 -0700 Subject: [PATCH] feat: generic marshmallow error component (#25303) --- .../Datasource/DatasourceModal.test.jsx | 2 +- .../components/Datasource/DatasourceModal.tsx | 19 ++- .../MarshmallowErrorMessage.test.tsx | 86 ++++++++++++++ .../ErrorMessage/MarshmallowErrorMessage.tsx | 109 ++++++++++++++++++ .../src/components/ErrorMessage/types.ts | 12 +- .../src/components/FilterableTable/index.tsx | 31 +---- .../src/hooks/useJsonTreeTheme.ts | 41 +++++++ .../src/setup/setupErrorMessages.ts | 5 + superset/datasets/api.py | 1 - superset/datasets/schemas.py | 12 ++ superset/errors.py | 3 + superset/exceptions.py | 17 +++ tests/unit_tests/datasets/api_tests.py | 73 ++++++++++++ 13 files changed, 376 insertions(+), 35 deletions(-) create mode 100644 superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.test.tsx create mode 100644 superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.tsx create mode 100644 superset-frontend/src/hooks/useJsonTreeTheme.ts create mode 100644 tests/unit_tests/datasets/api_tests.py diff --git a/superset-frontend/src/components/Datasource/DatasourceModal.test.jsx b/superset-frontend/src/components/Datasource/DatasourceModal.test.jsx index 5bcb705b683d4..24536cac003de 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal.test.jsx +++ b/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"]') diff --git a/superset-frontend/src/components/Datasource/DatasourceModal.tsx b/superset-frontend/src/components/Datasource/DatasourceModal.tsx index d4239136a7d1d..fed45b43f2e42 100644 --- a/superset-frontend/src/components/Datasource/DatasourceModal.tsx +++ b/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 = ({ }) .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: ( + + ), }); }); }); diff --git a/superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.test.tsx b/superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.test.tsx new file mode 100644 index 0000000000000..5fd6ee7bfbd44 --- /dev/null +++ b/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( + + + , + ); + expect(screen.getByText('Validation failed')).toBeInTheDocument(); + }); + + test('renders the provided subtitle', () => { + render( + + + , + ); + expect(screen.getByText('Error Alert')).toBeInTheDocument(); + }); + + test('renders extracted invalid values', () => { + render( + + + , + ); + 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( + + + , + ); + fireEvent.click(screen.getByText('Details')); + expect(screen.getByText('"can\'t be blank"')).toBeInTheDocument(); + }); +}); diff --git a/superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.tsx b/superset-frontend/src/components/ErrorMessage/MarshmallowErrorMessage.tsx new file mode 100644 index 0000000000000..854de2ee3904a --- /dev/null +++ b/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) { + const jsonTreeTheme = useJsonTreeTheme(); + const { extra, message } = error; + + return ( +
+ {subtitle &&

{subtitle}

} + + {message} + + + {extractInvalidValues(extra.messages, extra.payload).map( + (value, index) => ( +
  • {value}
    • + ), + )} +     + + + + true} + hideRoot + theme={jsonTreeTheme} + /> + + +
    + ); +} diff --git a/superset-frontend/src/components/ErrorMessage/types.ts b/superset-frontend/src/components/ErrorMessage/types.ts index d3fe5bfdf7aff..c32435d7f4fbe 100644 --- a/superset-frontend/src/components/ErrorMessage/types.ts +++ b/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[keyof T]; @@ -88,7 +89,7 @@ export type ErrorType = ValueOf; // 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 | null> = { error_type: ErrorType; @@ -106,3 +107,12 @@ export type ErrorMessageComponentProps | null> = export type ErrorMessageComponent = React.ComponentType; + +/* 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', +}); diff --git a/superset-frontend/src/components/FilterableTable/index.tsx b/superset-frontend/src/components/FilterableTable/index.tsx index 91fc1f4477f2c..b27bc330102ec 100644 --- a/superset-frontend/src/components/FilterableTable/index.tsx +++ b/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>(); - - 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={ } diff --git a/superset-frontend/src/hooks/useJsonTreeTheme.ts b/superset-frontend/src/hooks/useJsonTreeTheme.ts new file mode 100644 index 0000000000000..a052e22a63910 --- /dev/null +++ b/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, + }; +}; diff --git a/superset-frontend/src/setup/setupErrorMessages.ts b/superset-frontend/src/setup/setupErrorMessages.ts index bebb22b20c13e..59842f190adae 100644 --- a/superset-frontend/src/setup/setupErrorMessages.ts +++ b/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(); } diff --git a/superset/datasets/api.py b/superset/datasets/api.py index 5c722cc0df963..e1d7e5a09eb51 100644 --- a/superset/datasets/api.py +++ b/superset/datasets/api.py @@ -332,7 +332,6 @@ def post(self) -> Response: @expose("/", methods=("PUT",)) @protect() - @safe @statsd_metrics @event_logger.log_this_with_context( action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.put", diff --git a/superset/datasets/schemas.py b/superset/datasets/schemas.py index 0b137e96a7da9..84f0453fb4f46 100644 --- a/superset/datasets/schemas.py +++ b/superset/datasets/schemas.py @@ -24,6 +24,7 @@ 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) diff --git a/superset/errors.py b/superset/errors.py index a480ea3d28857..87cdf77b9944b 100644 --- a/superset/errors.py +++ b/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 @@ class SupersetErrorType(StrEnum): 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 @@ class SupersetErrorType(StrEnum): SupersetErrorType.ASYNC_WORKERS_ERROR: [1035], SupersetErrorType.DATABASE_NOT_FOUND_ERROR: [1011, 1036], SupersetErrorType.CONNECTION_DATABASE_TIMEOUT: [1001, 1009], + SupersetErrorType.MARSHMALLOW_ERROR: [1040], } diff --git a/superset/exceptions.py b/superset/exceptions.py index 5f1df73c86882..3642a9279ec23 100644 --- a/superset/exceptions.py +++ b/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) diff --git a/tests/unit_tests/datasets/api_tests.py b/tests/unit_tests/datasets/api_tests.py new file mode 100644 index 0000000000000..de93720fa60d0 --- /dev/null +++ b/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." + ), + } + ], + }, + } + ] + }