-
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
fix(sql lab): Syntax errors should return with 422 status #20491
Conversation
32c7e7d
to
cf64245
Compare
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.
See comment.
superset/sqllab/command.py
Outdated
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 |
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.
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.
Love this, could you also include a test for this?
9ec5a09
to
b2fe396
Compare
b2fe396
to
9fb87b1
Compare
The overall flow is covered by integration tests, but there's no command test setup for sqllab, and that setup could take a while. Will follow up with another pr for that one |
if all(ex.error_type == SupersetErrorType.SYNTAX_ERROR for ex in ex.errors): | ||
raise SupersetSyntaxErrorException(ex.errors) from ex | ||
raise ex | ||
except SupersetException as 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.
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 comment
The 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
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.
Thanks @diegomedina248!
SUMMARY
A syntax error in SQL Lab, like the following:
returns a 500 error.
This PR changes the status to a 422, to reflect it as an user error instead.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Execute a query with a syntax error in SQL Lab and observe the response status. Should be 422
ADDITIONAL INFORMATION