From 64e502a726f631755d6b76217fb78c5f3df9c56a Mon Sep 17 00:00:00 2001 From: hughhhh Date: Mon, 17 Oct 2022 14:14:23 -0400 Subject: [PATCH 1/9] if user isnt client log exception --- superset/sqllab/command.py | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index a8ee3c0934734..d3d7a04f3e412 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -123,20 +123,14 @@ def run( # pylint: disable=too-many-statements,useless-suppression "payload": self._execution_context_convertor.serialize_payload(), } except SupersetErrorsException as ex: - if all(ex.error_type in USER_CLIENT_ERRORS for ex in ex.errors): - raise SupersetSyntaxErrorException(ex.errors) from ex + if not all(ex.error_type in USER_CLIENT_ERRORS for ex in ex.errors): + query_id = query.id if query else None + logger.exception("Query %d: %s", query_id, type(ex)) raise ex except SupersetException as ex: - if ex.error_type in USER_CLIENT_ERRORS: - raise SupersetSyntaxErrorException( - [ - SupersetError( - message=ex.message, - error_type=ex.error_type, - level=ErrorLevel.ERROR, - ) - ] - ) from ex + if not ex.error_type in USER_CLIENT_ERRORS: + query_id = query.id if query else None + logger.exception("Query %d: %s", query_id, type(ex)) raise ex except Exception as ex: query_id = query.id if query else None From f9c8ab49fc59fd3a9768974e79693482579446b1 Mon Sep 17 00:00:00 2001 From: hughhhh Date: Mon, 17 Oct 2022 14:38:37 -0400 Subject: [PATCH 2/9] refactor --- superset/sqllab/command.py | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index d3d7a04f3e412..6ef215c23c0da 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -18,7 +18,7 @@ from __future__ import annotations import logging -from typing import Any, Dict, Optional, Set, TYPE_CHECKING +from typing import Any, Dict, Optional, Set, TYPE_CHECKING, Union from flask_babel import gettext as __ @@ -122,13 +122,8 @@ def run( # pylint: disable=too-many-statements,useless-suppression "status": status, "payload": self._execution_context_convertor.serialize_payload(), } - except SupersetErrorsException as ex: - if not all(ex.error_type in USER_CLIENT_ERRORS for ex in ex.errors): - query_id = query.id if query else None - logger.exception("Query %d: %s", query_id, type(ex)) - raise ex - except SupersetException as ex: - if not ex.error_type in USER_CLIENT_ERRORS: + except (SupersetErrorsException, SupersetException) as ex: + if ex.status == 422: query_id = query.id if query else None logger.exception("Query %d: %s", query_id, type(ex)) raise ex From 9276370ab8a2eab1e165c7fd314226c7df131d9e Mon Sep 17 00:00:00 2001 From: hughhhh Date: Mon, 17 Oct 2022 14:40:07 -0400 Subject: [PATCH 3/9] refactor --- superset/sqllab/command.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index 6ef215c23c0da..5d7a424b315cc 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -53,13 +53,6 @@ CommandResult = Dict[str, Any] -# Define set of users client errors these definitions won't -# be logged as exception vs. warning -USER_CLIENT_ERRORS: Set[SupersetErrorType] = { - SupersetErrorType.SYNTAX_ERROR, - SupersetErrorType.COLUMN_DOES_NOT_EXIST_ERROR, -} - class ExecuteSqlCommand(BaseCommand): _execution_context: SqlJsonExecutionContext From a792ae1b96885e26f5736b5b1e2abfa5bc19777f Mon Sep 17 00:00:00 2001 From: hughhhh Date: Mon, 17 Oct 2022 16:01:47 -0400 Subject: [PATCH 4/9] fix linting --- superset/sqllab/command.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index 5d7a424b315cc..135ce0f1fd07e 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -18,19 +18,18 @@ from __future__ import annotations import logging -from typing import Any, Dict, Optional, Set, TYPE_CHECKING, Union +from typing import Any, Dict, Optional, TYPE_CHECKING from flask_babel import gettext as __ from superset.commands.base import BaseCommand from superset.common.db_query_status import QueryStatus from superset.dao.exceptions import DAOCreateFailedError -from superset.errors import ErrorLevel, SupersetError, SupersetErrorType +from superset.errors import SupersetErrorType from superset.exceptions import ( SupersetErrorsException, SupersetException, SupersetGenericErrorException, - SupersetSyntaxErrorException, ) from superset.models.core import Database from superset.models.sql_lab import Query @@ -116,7 +115,7 @@ def run( # pylint: disable=too-many-statements,useless-suppression "payload": self._execution_context_convertor.serialize_payload(), } except (SupersetErrorsException, SupersetException) as ex: - if ex.status == 422: + if ex.status != 422: query_id = query.id if query else None logger.exception("Query %d: %s", query_id, type(ex)) raise ex From 67c3f2017685d38422df53a7479009d415ba5f24 Mon Sep 17 00:00:00 2001 From: hughhhh Date: Fri, 21 Oct 2022 12:05:40 -0700 Subject: [PATCH 5/9] curious --- superset/sqllab/command.py | 4 ++-- tests/integration_tests/celery_tests.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index 135ce0f1fd07e..e6319c2c7707a 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -116,11 +116,11 @@ def run( # pylint: disable=too-many-statements,useless-suppression } except (SupersetErrorsException, SupersetException) as ex: if ex.status != 422: - query_id = query.id if query else None + query_id = query.id if query else -1 logger.exception("Query %d: %s", query_id, type(ex)) raise ex except Exception as ex: - query_id = query.id if query else None + query_id = query.id if query else -1 logger.exception("Query %d: %s", query_id, type(ex)) raise SqlLabException(self._execution_context, exception=ex) from ex diff --git a/tests/integration_tests/celery_tests.py b/tests/integration_tests/celery_tests.py index f057d3128e574..c8a1f9d0ca70c 100644 --- a/tests/integration_tests/celery_tests.py +++ b/tests/integration_tests/celery_tests.py @@ -148,6 +148,7 @@ def test_run_sync_query_dont_exist(test_client, ctas_method): engine_name = examples_db.db_engine_spec.engine_name sql_dont_exist = "SELECT name FROM table_dont_exist" result = run_sql(test_client, sql_dont_exist, cta=True, ctas_method=ctas_method) + print(result) if backend() == "sqlite" and ctas_method == CtasMethod.VIEW: assert QueryStatus.SUCCESS == result["status"], result elif backend() == "presto": From a977e82bf2ebcb2df18b63b69da845a86b72a7a3 Mon Sep 17 00:00:00 2001 From: hughhhh Date: Fri, 21 Oct 2022 12:30:11 -0700 Subject: [PATCH 6/9] logging from upper pattern --- superset/sqllab/command.py | 6 +----- tests/integration_tests/celery_tests.py | 1 - 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index e6319c2c7707a..2a02b854aa26a 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -115,13 +115,9 @@ def run( # pylint: disable=too-many-statements,useless-suppression "payload": self._execution_context_convertor.serialize_payload(), } except (SupersetErrorsException, SupersetException) as ex: - if ex.status != 422: - query_id = query.id if query else -1 - logger.exception("Query %d: %s", query_id, type(ex)) + logger.warning("ExecuteSqlCommand failed with %s", str(ex)) raise ex except Exception as ex: - query_id = query.id if query else -1 - logger.exception("Query %d: %s", query_id, type(ex)) raise SqlLabException(self._execution_context, exception=ex) from ex def _try_get_existing_query(self) -> Optional[Query]: diff --git a/tests/integration_tests/celery_tests.py b/tests/integration_tests/celery_tests.py index c8a1f9d0ca70c..f057d3128e574 100644 --- a/tests/integration_tests/celery_tests.py +++ b/tests/integration_tests/celery_tests.py @@ -148,7 +148,6 @@ def test_run_sync_query_dont_exist(test_client, ctas_method): engine_name = examples_db.db_engine_spec.engine_name sql_dont_exist = "SELECT name FROM table_dont_exist" result = run_sql(test_client, sql_dont_exist, cta=True, ctas_method=ctas_method) - print(result) if backend() == "sqlite" and ctas_method == CtasMethod.VIEW: assert QueryStatus.SUCCESS == result["status"], result elif backend() == "presto": From ca13dac627644d9c0fbdbdc9e56ac3167a52b80e Mon Sep 17 00:00:00 2001 From: hughhhh Date: Tue, 25 Oct 2022 13:47:49 -0400 Subject: [PATCH 7/9] remove except --- superset/sqllab/command.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index 2a02b854aa26a..2d8224e19d971 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -114,9 +114,6 @@ def run( # pylint: disable=too-many-statements,useless-suppression "status": status, "payload": self._execution_context_convertor.serialize_payload(), } - except (SupersetErrorsException, SupersetException) as ex: - logger.warning("ExecuteSqlCommand failed with %s", str(ex)) - raise ex except Exception as ex: raise SqlLabException(self._execution_context, exception=ex) from ex From 93a2c6358df10c6a568be5f8e1e6fd5d8bff903f Mon Sep 17 00:00:00 2001 From: hughhhh Date: Fri, 28 Oct 2022 13:59:50 -0400 Subject: [PATCH 8/9] remove unused commands --- superset/sqllab/command.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index 2d8224e19d971..4c89c00de3f62 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -26,11 +26,7 @@ from superset.common.db_query_status import QueryStatus from superset.dao.exceptions import DAOCreateFailedError from superset.errors import SupersetErrorType -from superset.exceptions import ( - SupersetErrorsException, - SupersetException, - SupersetGenericErrorException, -) +from superset.exceptions import SupersetGenericErrorException from superset.models.core import Database from superset.models.sql_lab import Query from superset.sqllab.command_status import SqlJsonExecutionStatus From e7caecc9a588e53bab8fc4abed4f1af2dc1c839b Mon Sep 17 00:00:00 2001 From: hughhhh Date: Fri, 28 Oct 2022 15:36:24 -0400 Subject: [PATCH 9/9] put an extra except to make sure the original exception are kept --- superset/sqllab/command.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/superset/sqllab/command.py b/superset/sqllab/command.py index 4c89c00de3f62..35a761fab4018 100644 --- a/superset/sqllab/command.py +++ b/superset/sqllab/command.py @@ -26,7 +26,11 @@ from superset.common.db_query_status import QueryStatus from superset.dao.exceptions import DAOCreateFailedError from superset.errors import SupersetErrorType -from superset.exceptions import SupersetGenericErrorException +from superset.exceptions import ( + SupersetErrorException, + SupersetErrorsException, + SupersetGenericErrorException, +) from superset.models.core import Database from superset.models.sql_lab import Query from superset.sqllab.command_status import SqlJsonExecutionStatus @@ -110,6 +114,10 @@ def run( # pylint: disable=too-many-statements,useless-suppression "status": status, "payload": self._execution_context_convertor.serialize_payload(), } + except (SupersetErrorException, SupersetErrorsException) as ex: + # to make sure we raising the original + # SupersetErrorsException || SupersetErrorsException + raise ex except Exception as ex: raise SqlLabException(self._execution_context, exception=ex) from ex