You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by be...@apache.org on 2024/10/04 23:18:04 UTC
(superset) branch master updated: feat(SQL Lab): better SQL parsing error messages (#30501)
This is an automated email from the ASF dual-hosted git repository.
beto pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new a098809294 feat(SQL Lab): better SQL parsing error messages (#30501)
a098809294 is described below
commit a09880929482ef7ff93cd7bee124e56a9bb741b1
Author: Beto Dealmeida <ro...@dealmeida.net>
AuthorDate: Fri Oct 4 19:17:56 2024 -0400
feat(SQL Lab): better SQL parsing error messages (#30501)
---
.../src/SqlLab/components/ResultSet/index.tsx | 36 ++++--
.../ErrorMessage/InvalidSQLErrorMessage.test.tsx | 126 +++++++++++++++++++++
.../ErrorMessage/InvalidSQLErrorMessage.tsx | 62 ++++++++++
superset-frontend/src/setup/setupErrorMessages.ts | 5 +
superset/exceptions.py | 2 +-
superset/sql_lab.py | 9 +-
6 files changed, 226 insertions(+), 14 deletions(-)
diff --git a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx
index 067a2c3b09..3e9c9db5c6 100644
--- a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx
+++ b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx
@@ -42,6 +42,7 @@ import {
css,
getNumberFormatter,
getExtensionsRegistry,
+ ErrorLevel,
ErrorTypeEnum,
} from '@superset-ui/core';
import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
@@ -540,18 +541,33 @@ const ResultSet = ({
}
if (query.state === QueryState.Failed) {
+ const errors = [];
+ if (query.errorMessage) {
+ errors.push({
+ error_type: ErrorTypeEnum.GENERIC_DB_ENGINE_ERROR,
+ extra: {},
+ level: 'error' as ErrorLevel,
+ message: query.errorMessage,
+ });
+ }
+ errors.push(...(query.extra?.errors || []), ...(query.errors || []));
+
return (
<ResultlessStyles>
- <ErrorMessageWithStackTrace
- title={t('Database error')}
- error={query?.extra?.errors?.[0] || query?.errors?.[0]}
- subtitle={<MonospaceDiv>{query.errorMessage}</MonospaceDiv>}
- copyText={query.errorMessage || undefined}
- link={query.link}
- source="sqllab"
- />
- {(query?.extra?.errors?.[0] || query?.errors?.[0])?.error_type ===
- ErrorTypeEnum.FRONTEND_TIMEOUT_ERROR ? (
+ {errors.map((error, index) => (
+ <ErrorMessageWithStackTrace
+ key={index}
+ title={t('Database error')}
+ error={error}
+ subtitle={<MonospaceDiv>{error.message}</MonospaceDiv>}
+ copyText={error.message || undefined}
+ link={query.link}
+ source="sqllab"
+ />
+ ))}
+ {errors.some(
+ error => error?.error_type === ErrorTypeEnum.FRONTEND_TIMEOUT_ERROR,
+ ) ? (
<Button
className="sql-result-track-job"
buttonSize="small"
diff --git a/superset-frontend/src/components/ErrorMessage/InvalidSQLErrorMessage.test.tsx b/superset-frontend/src/components/ErrorMessage/InvalidSQLErrorMessage.test.tsx
new file mode 100644
index 0000000000..9a6953902f
--- /dev/null
+++ b/superset-frontend/src/components/ErrorMessage/InvalidSQLErrorMessage.test.tsx
@@ -0,0 +1,126 @@
+/**
+ * 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 } from '@testing-library/react';
+import '@testing-library/jest-dom/extend-expect';
+import {
+ ErrorLevel,
+ ErrorSource,
+ ErrorTypeEnum,
+ ThemeProvider,
+ supersetTheme,
+} from '@superset-ui/core';
+import InvalidSQLErrorMessage from './InvalidSQLErrorMessage';
+
+const defaultProps = {
+ error: {
+ error_type: ErrorTypeEnum.INVALID_SQL_ERROR,
+ message: 'Invalid SQL',
+ level: 'error' as ErrorLevel,
+ extra: {
+ sql: 'SELECT * FFROM table',
+ line: 1,
+ column: 10,
+ engine: 'postgresql',
+ },
+ },
+ source: 'test' as ErrorSource,
+ subtitle: 'Test subtitle',
+};
+
+const setup = (overrides = {}) => (
+ <ThemeProvider theme={supersetTheme}>
+ <InvalidSQLErrorMessage {...defaultProps} {...overrides} />;
+ </ThemeProvider>
+);
+
+// Mock the ErrorAlert component
+jest.mock('./ErrorAlert', () => ({
+ __esModule: true,
+ default: ({
+ title,
+ subtitle,
+ level,
+ source,
+ body,
+ }: {
+ title: React.ReactNode;
+ subtitle?: React.ReactNode;
+ level: ErrorLevel;
+ source: ErrorSource;
+ body: React.ReactNode;
+ }) => (
+ <div data-test="error-alert">
+ <div data-test="title">{title}</div>
+ <div data-test="subtitle">{subtitle}</div>
+ <div data-test="level">{level}</div>
+ <div data-test="source">{source}</div>
+ <div data-test="body">{body}</div>
+ </div>
+ ),
+}));
+
+describe('InvalidSQLErrorMessage', () => {
+ it('renders ErrorAlert with correct props', () => {
+ const { getByTestId } = render(setup());
+
+ expect(getByTestId('error-alert')).toBeInTheDocument();
+ expect(getByTestId('title')).toHaveTextContent('Unable to parse SQL');
+ expect(getByTestId('subtitle')).toHaveTextContent('Test subtitle');
+ expect(getByTestId('level')).toHaveTextContent('error');
+ expect(getByTestId('source')).toHaveTextContent('test');
+ });
+
+ it('displays the error line and column indicator', () => {
+ const { getByTestId } = render(setup());
+
+ const body = getByTestId('body');
+ expect(body).toContainHTML('<pre>SELECT * FFROM table</pre>');
+ expect(body).toContainHTML('<pre> ^</pre>');
+ });
+
+ it('handles missing line number', () => {
+ const { getByTestId } = render(
+ setup({
+ error: {
+ ...defaultProps.error,
+ extra: { ...defaultProps.error.extra, line: null },
+ },
+ }),
+ );
+
+ const body = getByTestId('body');
+ expect(body).toBeEmptyDOMElement();
+ });
+
+ it('handles missing column number', () => {
+ const { getByTestId } = render(
+ setup({
+ error: {
+ ...defaultProps.error,
+ extra: { ...defaultProps.error.extra, column: null },
+ },
+ }),
+ );
+
+ const body = getByTestId('body');
+ expect(body).toHaveTextContent('SELECT * FFROM table');
+ expect(body).not.toHaveTextContent('^');
+ });
+});
diff --git a/superset-frontend/src/components/ErrorMessage/InvalidSQLErrorMessage.tsx b/superset-frontend/src/components/ErrorMessage/InvalidSQLErrorMessage.tsx
new file mode 100644
index 0000000000..c7b701772d
--- /dev/null
+++ b/superset-frontend/src/components/ErrorMessage/InvalidSQLErrorMessage.tsx
@@ -0,0 +1,62 @@
+/**
+ * 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 { t } from '@superset-ui/core';
+import { ErrorMessageComponentProps } from './types';
+import ErrorAlert from './ErrorAlert';
+
+interface SupersetParseErrorExtra {
+ sql: string;
+ engine: string | null;
+ line: number | null;
+ column: number | null;
+}
+
+/*
+ * Component for showing syntax errors in SQL Lab.
+ */
+function InvalidSQLErrorMessage({
+ error,
+ source,
+ subtitle,
+}: ErrorMessageComponentProps<SupersetParseErrorExtra>) {
+ const { extra, level } = error;
+
+ const { sql, line, column } = extra;
+ const lines = sql.split('\n');
+ const errorLine = line !== null ? lines[line - 1] : null;
+ const body = errorLine && (
+ <>
+ <pre>{errorLine}</pre>
+ {column !== null && <pre>{' '.repeat(column - 1)}^</pre>}
+ </>
+ );
+
+ return (
+ <ErrorAlert
+ title={t('Unable to parse SQL')}
+ subtitle={subtitle}
+ level={level}
+ source={source}
+ body={body}
+ />
+ );
+}
+
+export default InvalidSQLErrorMessage;
diff --git a/superset-frontend/src/setup/setupErrorMessages.ts b/superset-frontend/src/setup/setupErrorMessages.ts
index 4d731cb4e3..442cd32152 100644
--- a/superset-frontend/src/setup/setupErrorMessages.ts
+++ b/superset-frontend/src/setup/setupErrorMessages.ts
@@ -23,6 +23,7 @@ import DatabaseErrorMessage from 'src/components/ErrorMessage/DatabaseErrorMessa
import MarshmallowErrorMessage from 'src/components/ErrorMessage/MarshmallowErrorMessage';
import ParameterErrorMessage from 'src/components/ErrorMessage/ParameterErrorMessage';
import DatasetNotFoundErrorMessage from 'src/components/ErrorMessage/DatasetNotFoundErrorMessage';
+import InvalidSQLErrorMessage from 'src/components/ErrorMessage/InvalidSQLErrorMessage';
import OAuth2RedirectMessage from 'src/components/ErrorMessage/OAuth2RedirectMessage';
import setupErrorMessagesExtra from './setupErrorMessagesExtra';
@@ -154,5 +155,9 @@ export default function setupErrorMessages() {
ErrorTypeEnum.OAUTH2_REDIRECT,
OAuth2RedirectMessage,
);
+ errorMessageComponentRegistry.registerValue(
+ ErrorTypeEnum.INVALID_SQL_ERROR,
+ InvalidSQLErrorMessage,
+ );
setupErrorMessagesExtra();
}
diff --git a/superset/exceptions.py b/superset/exceptions.py
index 492007523d..5afad60613 100644
--- a/superset/exceptions.py
+++ b/superset/exceptions.py
@@ -323,7 +323,7 @@ class SupersetParseError(SupersetErrorException):
if line:
parts.append(_(" at line %(line)d", line=line))
if column:
- parts.append(_(":%(column)d", column=column))
+ parts.append(f":{column}")
message = "".join(parts)
error = SupersetError(
diff --git a/superset/sql_lab.py b/superset/sql_lab.py
index 65a093610d..7685b4419c 100644
--- a/superset/sql_lab.py
+++ b/superset/sql_lab.py
@@ -238,15 +238,17 @@ def execute_sql_statement( # pylint: disable=too-many-statements, too-many-loca
increased_limit = None if query.limit is None else query.limit + 1
if not database.allow_dml:
+ errors = []
try:
parsed_statement = SQLStatement(sql_statement, engine=db_engine_spec.engine)
disallowed = parsed_statement.is_mutating()
- except SupersetParseError:
- # if we fail to parse teh query, disallow by default
+ except SupersetParseError as ex:
+ # if we fail to parse the query, disallow by default
disallowed = True
+ errors.append(ex.error)
if disallowed:
- raise SupersetErrorException(
+ errors.append(
SupersetError(
message=__(
"This database does not allow for DDL/DML, and the query "
@@ -257,6 +259,7 @@ def execute_sql_statement( # pylint: disable=too-many-statements, too-many-loca
level=ErrorLevel.ERROR,
)
)
+ raise SupersetErrorsException(errors)
if apply_ctas:
if not query.tmp_table_name: