-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
feat: show rich error messages on past failed queries #15158
Conversation
msg = data["error"] | ||
query = _session.query(Query).filter_by(id=query_id).one() | ||
database = query.database | ||
db_engine_spec = database.db_engine_spec | ||
errors = db_engine_spec.extract_errors(msg) | ||
_session.close() |
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 logic was moved to superset/sql_lab.py
if (latestQuery?.extra?.errors) { | ||
latestQuery.errors = latestQuery.extra.errors; | ||
} |
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 all that's needed to show the rich error messages in the FE.
from superset.utils import core as utils | ||
|
||
if TYPE_CHECKING: | ||
# prevent circular imports | ||
from superset.models.core import Database | ||
|
||
|
||
COLUMN_DOES_NOT_EXIST_REGEX = re.compile("no such column: (?P<column_name>.+)") |
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 was testing with gsheets, so I decided to add this to have a custom error to test.
Codecov Report
@@ Coverage Diff @@
## master #15158 +/- ##
=======================================
Coverage 77.41% 77.41%
=======================================
Files 969 969
Lines 49982 49988 +6
Branches 6422 6423 +1
=======================================
+ Hits 38692 38697 +5
- Misses 11087 11088 +1
Partials 203 203
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@@ -118,6 +118,9 @@ export default function SouthPane({ | |||
} | |||
let results; | |||
if (latestQuery) { | |||
if (latestQuery?.extra?.errors) { |
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.
nit:
if (latestQuery?.extra?.errors) { | |
const { extra: { errors } } = latestQuery; | |
if (errors) { | |
latestQuery.errors = errors; | |
} |
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 would fail if extra
is not there, no?
>> const { extra: { errors } } = { foo: "bar" };
Uncaught TypeError: ({foo:"bar"}).extra is undefined
Though I just realized that I could've used if (latestQuery.extra?.errors) {
instead, since we check if (latestQuery)
already.
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.
🚀
* feat: store SIP-40 error payload with queries * Set errors in query on load
* feat: store SIP-40 error payload with queries * Set errors in query on load
* feat: store SIP-40 error payload with queries * Set errors in query on load
SUMMARY
This PR makes SQL Lab show rich error messages when refreshing queries that failed.
The steps needed were:
superset/sql_lab.py
so that the SIP-40 error payload gets saved with the query in theextra_json
column under theerrors
key.query.extra.errors
toquery.errors
, so that errors are shown with the rich error component.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before, when refreshing a failed query we'd see:
After, when we refresh the page we see:
TESTING INSTRUCTIONS
Tested manually with sync and async queries.
ADDITIONAL INFORMATION