-
Notifications
You must be signed in to change notification settings - Fork 14k
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: Avoid 500 if end users write bad SQL #26638
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #26638 +/- ##
==========================================
- Coverage 69.18% 69.06% -0.13%
==========================================
Files 1948 1938 -10
Lines 76036 75596 -440
Branches 8478 8478
==========================================
- Hits 52604 52208 -396
+ Misses 21265 21221 -44
Partials 2167 2167
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
We have lots of these unnecessary 500s, which makes logs noisy, thanks for cleaning these up 👍 While the new Trino exception mappings are probably very uncontroversial, I'd like a few extra eyes on changing the status type of SupersetDBAPIProgrammingError
. However, I feel 400 is probably a better default than 500 for that one.
I've initially submitted Draft PR to see at least some feedback. The situation there is a bit more complex and I've pushed more code.
I don't know how to interpret 500 errors with connection to underlying source Databases... 🤔 |
(cherry picked from commit 80a6e25)
(cherry picked from commit 80a6e25)
SUMMARY
While end users deal with Superset+Trino we are observing quite some 500 errors on backend.
For example, a user can create/edit a virtual dataset and make mistakes in SQL. Trino client raises the exception and it goes uncaught to 500.
If SupersetDBAPIProgrammingError is raised it's better to return 400 to the end user because nothing bad happens with Superset instance.
I don't know what to do with
SupersetDBAPIError
in general. If underlying Database has problems, should it lead to Superset's 500 or it's ok to stay with 400 because Superset itself works fine.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION