-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
fix(sql lab): Syntax errors should return with 422 status #20491
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 SupersetErrorsException, SupersetGenericErrorException | ||
from superset.exceptions import ( | ||
SupersetErrorsException, | ||
SupersetException, | ||
SupersetGenericErrorException, | ||
) | ||
from superset.models.core import Database | ||
from superset.models.sql_lab import Query | ||
from superset.sqllab.command_status import SqlJsonExecutionStatus | ||
|
@@ -110,7 +114,13 @@ def run( # pylint: disable=too-many-statements,useless-suppression | |
"status": status, | ||
"payload": self._execution_context_convertor.serialize_payload(), | ||
} | ||
except (SqlLabException, SupersetErrorsException) as ex: | ||
except SupersetErrorsException as ex: | ||
if all(ex.error_type == SupersetErrorType.SYNTAX_ERROR for ex in ex.errors): | ||
ex.status = 422 | ||
raise ex | ||
except SupersetException as ex: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that we were catching SqlLabException before. This one is a parent class, correct, so it should still work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, correct, and most likely that exception will be thrown as is, unless it matches the criteria |
||
if ex.error_type == SupersetErrorType.SYNTAX_ERROR: | ||
ex.status = 422 | ||
raise ex | ||
except Exception as ex: | ||
raise SqlLabException(self._execution_context, exception=ex) from ex | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great @diegomedina248. Thank you! Can we raise a new error instead of the original one? We may have to create a new syntax error class with a 422 status.