From a09880929482ef7ff93cd7bee124e56a9bb741b1 Mon Sep 17 00:00:00 2001
From: Beto Dealmeida <roberto@dealmeida.net>
Date: Fri, 4 Oct 2024 19:17:56 -0400
Subject: [PATCH] feat(SQL Lab): better SQL parsing error messages (#30501)

---
 .../src/SqlLab/components/ResultSet/index.tsx |  36 +++--
 .../InvalidSQLErrorMessage.test.tsx           | 126 ++++++++++++++++++
 .../ErrorMessage/InvalidSQLErrorMessage.tsx   |  62 +++++++++
 .../src/setup/setupErrorMessages.ts           |   5 +
 superset/exceptions.py                        |   2 +-
 superset/sql_lab.py                           |   9 +-
 6 files changed, 226 insertions(+), 14 deletions(-)
 create mode 100644 superset-frontend/src/components/ErrorMessage/InvalidSQLErrorMessage.test.tsx
 create mode 100644 superset-frontend/src/components/ErrorMessage/InvalidSQLErrorMessage.tsx

diff --git a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx
index 067a2c3b0958e..3e9c9db5c6d48 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 0000000000000..9a6953902f6ef
--- /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 0000000000000..c7b701772d429
--- /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 4d731cb4e392d..442cd32152bc9 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 492007523de3f..5afad60613d67 100644
--- a/superset/exceptions.py
+++ b/superset/exceptions.py
@@ -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(
diff --git a/superset/sql_lab.py b/superset/sql_lab.py
index 65a093610d114..7685b4419ce37 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: