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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 11 commits into from
Oct 31, 2022
Merged

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

Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
64e502a
if user isnt client log exception
hughhhh Oct 17, 2022
f9c8ab4
refactor
hughhhh Oct 17, 2022
9276370
refactor
hughhhh Oct 17, 2022
a792ae1
fix linting
hughhhh Oct 17, 2022
04bec5c
Merge branch 'master' of https://github.com/apache/superset into ret-ex
hughhhh Oct 21, 2022
67c3f20
curious
hughhhh Oct 21, 2022
a977e82
logging from upper pattern
hughhhh Oct 21, 2022
ca13dac
remove except
hughhhh Oct 25, 2022
93a2c63
remove unused commands
hughhhh Oct 28, 2022
f252333
Merge branch 'master' of https://github.com/apache/superset into ret-ex
hughhhh Oct 28, 2022
e7caecc
put an extra except to make sure the original exception are kept
hughhhh Oct 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 3 additions & 33 deletions superset/sqllab/command.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,15 @@
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.exceptions import (
SupersetErrorsException,
SupersetException,
SupersetGenericErrorException,
SupersetSyntaxErrorException,
)
from superset.errors import SupersetErrorType
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
Expand All @@ -53,13 +48,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 +110,7 @@ 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
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 (

superset/superset/views/base.py

Line 202 in 9edb581

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