Skip to content

Commit

Permalink
feat(SQL Lab): better SQL parsing error messages (apache#30501)
Browse files Browse the repository at this point in the history
  • Loading branch information
betodealmeida authored Oct 4, 2024
1 parent 95325c4 commit a098809
Show file tree
Hide file tree
Showing 6 changed files with 226 additions and 14 deletions.
36 changes: 26 additions & 10 deletions superset-frontend/src/SqlLab/components/ResultSet/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import {
css,
getNumberFormatter,
getExtensionsRegistry,
ErrorLevel,
ErrorTypeEnum,
} from '@superset-ui/core';
import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
Expand Down Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
@@ -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('^');
});
});
Original file line number Diff line number Diff line change
@@ -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;
5 changes: 5 additions & 0 deletions superset-frontend/src/setup/setupErrorMessages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -154,5 +155,9 @@ export default function setupErrorMessages() {
ErrorTypeEnum.OAUTH2_REDIRECT,
OAuth2RedirectMessage,
);
errorMessageComponentRegistry.registerValue(
ErrorTypeEnum.INVALID_SQL_ERROR,
InvalidSQLErrorMessage,
);
setupErrorMessagesExtra();
}
2 changes: 1 addition & 1 deletion superset/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,7 @@ def __init__( # pylint: disable=too-many-arguments
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(
Expand Down
9 changes: 6 additions & 3 deletions superset/sql_lab.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand All @@ -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:
Expand Down

0 comments on commit a098809

Please sign in to comment.