Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: return initial exception and check if it's user error #21836

Merged
merged 11 commits into from
Oct 31, 2022
34 changes: 6 additions & 28 deletions superset/sqllab/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,18 @@
from __future__ import annotations

import logging
from typing import Any, Dict, Optional, Set, TYPE_CHECKING
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 (
SupersetErrorException,
SupersetErrorsException,
SupersetException,
SupersetGenericErrorException,
SupersetSyntaxErrorException,
)
from superset.models.core import Database
from superset.models.sql_lab import Query
Expand All @@ -53,13 +52,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
Expand Down Expand Up @@ -122,25 +114,11 @@ def run( # pylint: disable=too-many-statements,useless-suppression
"status": status,
"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
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
except (SupersetErrorException, SupersetErrorsException) as ex:
# to make sure we raising the original
# SupersetErrorsException || SupersetErrorsException
raise ex
except Exception as ex:
query_id = query.id if query else None
logger.exception("Query %d: %s", query_id, type(ex))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it be useful to still log query id here or no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think having the id helps us debug the issue at all tbh, with that information we can't do anything else.

the important thing is that we log the exception here (

def handle_api_exception(
) once it gets bubbled up

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it!

raise SqlLabException(self._execution_context, exception=ex) from ex

def _try_get_existing_query(self) -> Optional[Query]:
Expand Down