From 4b00c152ccdd13284407d65107bdea6db33d100a Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 23 Jun 2021 07:58:20 -0700 Subject: [PATCH] feat: implement specific errors for SQL Lab (#15206) * RESULTS_BACKEND_NOT_CONFIGURED_ERROR * DML_NOT_ALLOWED_ERROR * INVALID_CxAS_QUERY_ERROR * Fix lint * Add more tests --- .../pages/docs/Miscellaneous/issue_codes.mdx | 40 ++++++ .../src/components/ErrorMessage/types.ts | 4 + .../src/setup/setupErrorMessages.ts | 16 +++ superset/errors.py | 45 +++++++ superset/sql_lab.py | 85 +++++++++---- superset/views/core.py | 12 +- tests/sqllab_tests.py | 120 ++++++++++++++++-- 7 files changed, 281 insertions(+), 41 deletions(-) diff --git a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx index ad26c70a7..cfe44493f 100644 --- a/docs/src/pages/docs/Miscellaneous/issue_codes.mdx +++ b/docs/src/pages/docs/Miscellaneous/issue_codes.mdx @@ -207,3 +207,43 @@ The submitted payload has the incorrect schema. ``` Please check that the request payload has the expected schema. + +## Issue 1021 + +``` +Results backend needed for asynchronous queries is not configured. +``` + +Your instance of Superset doesn't have a results backend configured, which is needed for asynchronous queries. Please contact an administrator for further assistance. + +## Issue 1022 + +``` +Database does not allow data manipulation. +``` + +Only `SELECT` statements are allowed against this database. Please contact an administrator if you need to run DML (data manipulation language) on this database. + +## Issue 1023 + +``` +CTAS (create table as select) doesn't have a SELECT statement at the end. +``` + +The last statement in a query run as CTAS (create table as select) MUST be a SELECT statement. Please make sure the last statement in the query is a SELECT. + +## Issue 1024 + +``` +CVAS (create view as select) query has more than one statement. +``` + +When running a CVAS (create view as select) the query should have a single statement. Please make sure the query has a single statement, and no extra semi-colons other than the last one. + +## Issue 1025 + +``` +CVAS (create view as select) query is not a SELECT statement. +``` + +When running a CVAS (create view as select) the query should be a SELECT statement. Please make sure the query has a single statement and it's a SELECT statement. diff --git a/superset-frontend/src/components/ErrorMessage/types.ts b/superset-frontend/src/components/ErrorMessage/types.ts index 5c345e04d..c277c59c5 100644 --- a/superset-frontend/src/components/ErrorMessage/types.ts +++ b/superset-frontend/src/components/ErrorMessage/types.ts @@ -57,6 +57,10 @@ export const ErrorTypeEnum = { // Sqllab error MISSING_TEMPLATE_PARAMS_ERROR: 'MISSING_TEMPLATE_PARAMS_ERROR', + RESULTS_BACKEND_NOT_CONFIGURED_ERROR: 'RESULTS_BACKEND_NOT_CONFIGURED_ERROR', + DML_NOT_ALLOWED_ERROR: 'DML_NOT_ALLOWED_ERROR', + INVALID_CTAS_QUERY_ERROR: 'INVALID_CTAS_QUERY_ERROR', + INVALID_CVAS_QUERY_ERROR: 'INVALID_CVAS_QUERY_ERROR', // Generic errors GENERIC_COMMAND_ERROR: 'GENERIC_COMMAND_ERROR', diff --git a/superset-frontend/src/setup/setupErrorMessages.ts b/superset-frontend/src/setup/setupErrorMessages.ts index ed244614d..6c005e97a 100644 --- a/superset-frontend/src/setup/setupErrorMessages.ts +++ b/superset-frontend/src/setup/setupErrorMessages.ts @@ -51,6 +51,22 @@ export default function setupErrorMessages() { ErrorTypeEnum.MISSING_TEMPLATE_PARAMS_ERROR, ParameterErrorMessage, ); + errorMessageComponentRegistry.registerValue( + ErrorTypeEnum.RESULTS_BACKEND_NOT_CONFIGURED_ERROR, + DatabaseErrorMessage, + ); + errorMessageComponentRegistry.registerValue( + ErrorTypeEnum.DML_NOT_ALLOWED_ERROR, + DatabaseErrorMessage, + ); + errorMessageComponentRegistry.registerValue( + ErrorTypeEnum.INVALID_CTAS_QUERY_ERROR, + DatabaseErrorMessage, + ); + errorMessageComponentRegistry.registerValue( + ErrorTypeEnum.INVALID_CVAS_QUERY_ERROR, + DatabaseErrorMessage, + ); errorMessageComponentRegistry.registerValue( ErrorTypeEnum.CONNECTION_INVALID_HOSTNAME_ERROR, DatabaseErrorMessage, diff --git a/superset/errors.py b/superset/errors.py index 961cca444..92ba980f4 100644 --- a/superset/errors.py +++ b/superset/errors.py @@ -66,6 +66,10 @@ class SupersetErrorType(str, Enum): # Sql Lab errors MISSING_TEMPLATE_PARAMS_ERROR = "MISSING_TEMPLATE_PARAMS_ERROR" + RESULTS_BACKEND_NOT_CONFIGURED_ERROR = "RESULTS_BACKEND_NOT_CONFIGURED_ERROR" + DML_NOT_ALLOWED_ERROR = "DML_NOT_ALLOWED_ERROR" + INVALID_CTAS_QUERY_ERROR = "INVALID_CTAS_QUERY_ERROR" + INVALID_CVAS_QUERY_ERROR = "INVALID_CVAS_QUERY_ERROR" # Generic errors GENERIC_COMMAND_ERROR = "GENERIC_COMMAND_ERROR" @@ -147,6 +151,21 @@ ERROR_TYPES_TO_ISSUE_CODES_MAPPING = { ), }, ], + SupersetErrorType.RESULTS_BACKEND_NOT_CONFIGURED_ERROR: [ + { + "code": 1021, + "message": _( + "Issue 1021 - Results backend needed for asynchronous queries " + "is not configured." + ), + }, + ], + SupersetErrorType.DML_NOT_ALLOWED_ERROR: [ + { + "code": 1022, + "message": _("Issue 1022 - Database does not allow data manipulation."), + }, + ], SupersetErrorType.CONNECTION_INVALID_HOSTNAME_ERROR: [ { "code": 1007, @@ -250,6 +269,32 @@ ERROR_TYPES_TO_ISSUE_CODES_MAPPING = { ), } ], + SupersetErrorType.INVALID_CTAS_QUERY_ERROR: [ + { + "code": 1023, + "message": _( + "Issue 1023 - The CTAS (create table as select) doesn't have a " + "SELECT statement at the end. Please make sure your query has a " + "SELECT as its last statement. Then, try running your query again." + ), + }, + ], + SupersetErrorType.INVALID_CVAS_QUERY_ERROR: [ + { + "code": 1024, + "message": _( + "Issue 1024 - CVAS (create view as select) query has more than " + "one statement." + ), + }, + { + "code": 1025, + "message": _( + "Issue 1025 - CVAS (create view as select) query is not a " + "SELECT statement." + ), + }, + ], } diff --git a/superset/sql_lab.py b/superset/sql_lab.py index e6f3e5428..7fa5de203 100644 --- a/superset/sql_lab.py +++ b/superset/sql_lab.py @@ -28,13 +28,15 @@ import pyarrow as pa import simplejson as json from celery import Task from celery.exceptions import SoftTimeLimitExceeded -from flask_babel import lazy_gettext as _ +from flask_babel import gettext as __, lazy_gettext as _ from sqlalchemy.orm import Session from werkzeug.local import LocalProxy from superset import app, results_backend, results_backend_use_msgpack, security_manager from superset.dataframe import df_to_records from superset.db_engine_specs import BaseEngineSpec +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import SupersetErrorException, SupersetErrorsException from superset.extensions import celery_app from superset.models.core import Database from superset.models.sql_lab import LimitingFactor, Query @@ -86,25 +88,34 @@ class SqlLabTimeoutException(SqlLabException): def handle_query_error( - msg: str, query: Query, session: Session, payload: Optional[Dict[str, Any]] = None + ex: Exception, + query: Query, + session: Session, + payload: Optional[Dict[str, Any]] = None, + prefix_message: str = "", ) -> Dict[str, Any]: """Local method handling error while processing the SQL""" payload = payload or {} + msg = f"{prefix_message} {str(ex)}".strip() troubleshooting_link = config["TROUBLESHOOTING_LINK"] query.error_message = msg query.status = QueryStatus.FAILED query.tmp_table_name = None # extract DB-specific errors (invalid column, eg) - errors = [ - dataclasses.asdict(error) - for error in query.database.db_engine_spec.extract_errors(msg) - ] + if isinstance(ex, SupersetErrorException): + errors = [ex.error] + elif isinstance(ex, SupersetErrorsException): + errors = ex.errors + else: + errors = query.database.db_engine_spec.extract_errors(str(ex)) + + errors_payload = [dataclasses.asdict(error) for error in errors] if errors: - query.set_extra_json_key("errors", errors) + query.set_extra_json_key("errors", errors_payload) session.commit() - payload.update({"status": query.status, "error": msg, "errors": errors}) + payload.update({"status": query.status, "error": msg, "errors": errors_payload}) if troubleshooting_link: payload["link"] = troubleshooting_link return payload @@ -186,7 +197,7 @@ def get_sql_results( # pylint: disable=too-many-arguments logger.debug("Query %d: %s", query_id, ex) stats_logger.incr("error_sqllab_unhandled") query = get_query(query_id, session) - return handle_query_error(str(ex), query, session) + return handle_query_error(ex, query, session) # pylint: disable=too-many-arguments, too-many-locals @@ -210,8 +221,12 @@ def execute_sql_statement( increased_limit = None if query.limit is None else query.limit + 1 if not db_engine_spec.is_readonly_query(parsed_query) and not database.allow_dml: - raise SqlLabSecurityException( - _("Only `SELECT` statements are allowed against this database") + raise SupersetErrorException( + SupersetError( + message=__("Only SELECT statements are allowed against this database."), + error_type=SupersetErrorType.DML_NOT_ALLOWED_ERROR, + level=ErrorLevel.ERROR, + ) ) if apply_ctas: if not query.tmp_table_name: @@ -353,7 +368,13 @@ def execute_sql_statements( # pylint: disable=too-many-arguments, too-many-loca db_engine_spec.patch() if database.allow_run_async and not results_backend: - raise SqlLabException("Results backend isn't configured.") + raise SupersetErrorException( + SupersetError( + message=__("Results backend is not configured."), + error_type=SupersetErrorType.RESULTS_BACKEND_NOT_CONFIGURED_ERROR, + level=ErrorLevel.ERROR, + ) + ) # Breaking down into multiple statements parsed_query = ParsedQuery(rendered_query, strip_comments=True) @@ -377,11 +398,16 @@ def execute_sql_statements( # pylint: disable=too-many-arguments, too-many-loca and query.ctas_method == CtasMethod.TABLE and not parsed_query.is_valid_ctas() ): - raise SqlLabException( - _( - "CTAS (create table as select) can only be run with a query where " - "the last statement is a SELECT. Please make sure your query has " - "a SELECT as its last statement. Then, try running your query again." + raise SupersetErrorException( + SupersetError( + message=__( + "CTAS (create table as select) can only be run with a query where " + "the last statement is a SELECT. Please make sure your query has " + "a SELECT as its last statement. Then, try running your query " + "again." + ), + error_type=SupersetErrorType.INVALID_CTAS_QUERY_ERROR, + level=ErrorLevel.ERROR, ) ) if ( @@ -389,11 +415,15 @@ def execute_sql_statements( # pylint: disable=too-many-arguments, too-many-loca and query.ctas_method == CtasMethod.VIEW and not parsed_query.is_valid_cvas() ): - raise SqlLabException( - _( - "CVAS (create view as select) can only be run with a query with " - "a single SELECT statement. Please make sure your query has only " - "a SELECT statement. Then, try running your query again." + raise SupersetErrorException( + SupersetError( + message=__( + "CVAS (create view as select) can only be run with a query with " + "a single SELECT statement. Please make sure your query has only " + "a SELECT statement. Then, try running your query again." + ), + error_type=SupersetErrorType.INVALID_CVAS_QUERY_ERROR, + level=ErrorLevel.ERROR, ) ) @@ -438,9 +468,14 @@ def execute_sql_statements( # pylint: disable=too-many-arguments, too-many-loca ) except Exception as ex: # pylint: disable=broad-except msg = str(ex) - if statement_count > 1: - msg = f"[Statement {i+1} out of {statement_count}] " + msg - payload = handle_query_error(msg, query, session, payload) + prefix_message = ( + f"[Statement {i+1} out of {statement_count}]" + if statement_count > 1 + else "" + ) + payload = handle_query_error( + ex, query, session, payload, prefix_message + ) return payload # Commit the connection so CTA queries will create the table. diff --git a/superset/views/core.py b/superset/views/core.py index 6efda998c..f86f5c9ee 100755 --- a/superset/views/core.py +++ b/superset/views/core.py @@ -73,12 +73,13 @@ from superset.dashboards.dao import DashboardDAO from superset.databases.dao import DatabaseDAO from superset.databases.filters import DatabaseFilter from superset.datasets.commands.exceptions import DatasetNotFoundError -from superset.errors import SupersetError +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType from superset.exceptions import ( CacheLoadError, CertificateException, DatabaseNotFound, SerializationError, + SupersetErrorException, SupersetErrorsException, SupersetException, SupersetGenericDBErrorException, @@ -2179,7 +2180,13 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods of rows returned. """ if not results_backend: - return json_error_response("Results backend isn't configured") + raise SupersetErrorException( + SupersetError( + message=__("Results backend is not configured."), + error_type=SupersetErrorType.RESULTS_BACKEND_NOT_CONFIGURED_ERROR, + level=ErrorLevel.ERROR, + ) + ) read_from_results_backend_start = now_as_float() blob = results_backend.get(key) @@ -2460,7 +2467,6 @@ class Superset(BaseSupersetView): # pylint: disable=too-many-public-methods raise SupersetErrorsException( [SupersetError(**params) for params in data["errors"]] ) - # old string-only error message raise SupersetGenericDBErrorException(data["error"]) diff --git a/tests/sqllab_tests.py b/tests/sqllab_tests.py index 07a2b4f3f..35d6f2ded 100644 --- a/tests/sqllab_tests.py +++ b/tests/sqllab_tests.py @@ -29,7 +29,8 @@ import prison from superset import db, security_manager from superset.connectors.sqla.models import SqlaTable from superset.db_engine_specs import BaseEngineSpec -from superset.errors import ErrorLevel, SupersetErrorType +from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.exceptions import SupersetErrorException from superset.models.core import Database from superset.models.sql_lab import LimitingFactor, Query, SavedQuery from superset.result_set import SupersetResultSet @@ -120,6 +121,29 @@ class TestSqlLab(SupersetTestCase): "engine_name": engine_name, } + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") + def test_sql_json_dml_disallowed(self): + self.login("admin") + + data = self.run_sql("DELETE FROM birth_names", "1") + assert data == { + "errors": [ + { + "message": "Only SELECT statements are allowed against this database.", + "error_type": SupersetErrorType.DML_NOT_ALLOWED_ERROR, + "level": ErrorLevel.ERROR, + "extra": { + "issue_codes": [ + { + "code": 1022, + "message": "Issue 1022 - Database does not allow data manipulation.", + } + ] + }, + } + ] + } + @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices") def test_sql_json_to_saved_query_info(self): """ @@ -744,6 +768,58 @@ class TestSqlLab(SupersetTestCase): ] ) + @mock.patch("superset.sql_lab.results_backend", None) + @mock.patch("superset.sql_lab.get_query") + @mock.patch("superset.sql_lab.execute_sql_statement") + def test_execute_sql_statements_no_results_backend( + self, mock_execute_sql_statement, mock_get_query + ): + sql = """ + -- comment + SET @value = 42; + SELECT @value AS foo; + -- comment + """ + mock_session = mock.MagicMock() + mock_query = mock.MagicMock() + mock_query.database.allow_run_async = True + mock_cursor = mock.MagicMock() + mock_query.database.get_sqla_engine.return_value.raw_connection.return_value.cursor.return_value = ( + mock_cursor + ) + mock_query.database.db_engine_spec.run_multiple_statements_as_one = False + mock_get_query.return_value = mock_query + + with pytest.raises(SupersetErrorException) as excinfo: + execute_sql_statements( + query_id=1, + rendered_query=sql, + return_results=True, + store_results=False, + user_name="admin", + session=mock_session, + start_time=None, + expand_data=False, + log_params=None, + ) + + assert excinfo.value.error == SupersetError( + message="Results backend is not configured.", + error_type=SupersetErrorType.RESULTS_BACKEND_NOT_CONFIGURED_ERROR, + level=ErrorLevel.ERROR, + extra={ + "issue_codes": [ + { + "code": 1021, + "message": ( + "Issue 1021 - Results backend needed for asynchronous " + "queries is not configured." + ), + } + ] + }, + ) + @mock.patch("superset.sql_lab.get_query") @mock.patch("superset.sql_lab.execute_sql_statement") def test_execute_sql_statements_ctas( @@ -805,7 +881,7 @@ class TestSqlLab(SupersetTestCase): # try invalid CTAS sql = "DROP TABLE my_table" - with pytest.raises(SqlLabException) as excinfo: + with pytest.raises(SupersetErrorException) as excinfo: execute_sql_statements( query_id=1, rendered_query=sql, @@ -817,11 +893,18 @@ class TestSqlLab(SupersetTestCase): expand_data=False, log_params=None, ) - assert str(excinfo.value) == ( - "CTAS (create table as select) can only be run with " - "a query where the last statement is a SELECT. Please " - "make sure your query has a SELECT as its last " - "statement. Then, try running your query again." + assert excinfo.value.error == SupersetError( + message="CTAS (create table as select) can only be run with a query where the last statement is a SELECT. Please make sure your query has a SELECT as its last statement. Then, try running your query again.", + error_type=SupersetErrorType.INVALID_CTAS_QUERY_ERROR, + level=ErrorLevel.ERROR, + extra={ + "issue_codes": [ + { + "code": 1023, + "message": "Issue 1023 - The CTAS (create table as select) doesn't have a SELECT statement at the end. Please make sure your query has a SELECT as its last statement. Then, try running your query again.", + } + ] + }, ) # try invalid CVAS @@ -832,7 +915,7 @@ class TestSqlLab(SupersetTestCase): SELECT @value AS foo; -- comment """ - with pytest.raises(SqlLabException) as excinfo: + with pytest.raises(SupersetErrorException) as excinfo: execute_sql_statements( query_id=1, rendered_query=sql, @@ -844,11 +927,22 @@ class TestSqlLab(SupersetTestCase): expand_data=False, log_params=None, ) - assert str(excinfo.value) == ( - "CVAS (create view as select) can only be run with a " - "query with a single SELECT statement. Please make " - "sure your query has only a SELECT statement. Then, " - "try running your query again." + assert excinfo.value.error == SupersetError( + message="CVAS (create view as select) can only be run with a query with a single SELECT statement. Please make sure your query has only a SELECT statement. Then, try running your query again.", + error_type=SupersetErrorType.INVALID_CVAS_QUERY_ERROR, + level=ErrorLevel.ERROR, + extra={ + "issue_codes": [ + { + "code": 1024, + "message": "Issue 1024 - CVAS (create view as select) query has more than one statement.", + }, + { + "code": 1025, + "message": "Issue 1025 - CVAS (create view as select) query is not a SELECT statement.", + }, + ] + }, ) @mock.patch("superset.sql_lab.get_query")