fix(sqllab): explain dataset save failures instead of "Fatal error" - #43334
fix(sqllab): explain dataset save failures instead of "Fatal error"#43334EnxDev wants to merge 2 commits into
Conversation
dosubot
Bot
added
change:backend
Code Review Agent Run #8602caActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43334 +/- ##
=======================================
Coverage 66.67% 66.67%
=======================================
Files 2876 2876
Lines 164007 164021 +14
Branches 37834 37842 +8
=======================================
+ Hits 109347 109357 +10
- Misses 52514 52517 +3
- Partials 2146 2147 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The current implementation relies solely on To resolve this, you should invalidate the successful-run state when the SQL changes. You can achieve this by tracking the SQL string associated with the successful query and comparing it to the current editor SQL, or by resetting the Here is a concise implementation to invalidate the state when the SQL changes: // In superset-frontend/src/SqlLab/components/SaveQuery/index.tsx
// 1. Track the SQL of the last successful run
const [lastSuccessfulSql, setLastSuccessfulSql] = useState<string | null>(null);
// 2. Update the tracker when the query state becomes success
useEffect(() => {
if (latestQueryState === QueryState.Success) {
setLastSuccessfulSql(queryEditor.sql);
}
}, [latestQueryState, queryEditor.sql]);
// 3. Update the success check to verify the SQL matches
const hasSuccessfulQuery =
latestQueryState === QueryState.Success &&
lastSuccessfulSql === queryEditor.sql;I have checked the PR comments, and there are no other comments in the provided context. Would you like me to perform any further analysis or address other parts of the PR? superset-frontend/src/SqlLab/components/SaveQuery/index.tsx |
Code Review Agent Run #c10eacActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
1 participant
SUMMARY
Saving a SQL Lab query as a dataset surfaced a red Fatal error toast with no detail, whatever the actual problem was; invalid SQL, a missing table, no permissions.
CreateDatasetCommand.run()introspects the new dataset's columns by executing its SQL against the analytics database.For a user with blanket database access that is the first time the query is parsed at all, since
validate()'s parse only runs on the per-table authorization branch. Failures there raiseSupersetGenericDBErrorException, which is not aSQLAlchemyError, soon_errorre-raised it untouched,DatasetRestApi.posthad no matchingexcept, and it reached FAB's@safeas a bare500 {"message": "Fatal error"};"Fatal error"being FAB's hardcoded generic 500 string, which the frontend then showed verbatim.Two changes, covering different failures:
1. The save now fails with a usable message.
CreateDatasetCommandconverts metadata-fetch failures into aDatasetInvalidError(422) carrying the engine's own text:SELECT ...422 {"sql": ["Invalid SQL: Error parsing near '.' at line 1:8"]}SELECT * FROM no_such_table422 {"sql": ["no such table: no_such_table"]}SELECT no_such_col FROM birth_names422 {"sql": ["no such column: no_such_col"]}DROP TABLE birth_names422 {"sql": ["Only \SELECT` statements are allowed"]}`The transaction still rolls back, so a failed save leaves no half-built dataset behind.
2. "Save dataset" is disabled until the query has run successfully, with the same tooltip wording used by "Schedule query"; mirroring the fix direction agreed for the sibling issue.
The gate alone is not enough. It only knows whether a query ran, not whether the editor's current text is what ran: run
SELECT 1, edit toSELECT * FROM no_such_tablewithout re-running, and the button stays enabled.Same for a table dropped between run and save, revoked permissions, or Jinja that renders differently at save time. Those all still reach the backend, which is why both halves ship together.
Also fixes a silent failure in
SaveDatasetModal: the error toast in thecatchwas constructed but never dispatched, so a failure in the chart-payload step showed the user nothing at all.Note on
[str(ex.message)]: these messages are built withlazy_gettext, and marshmallow only auto-wrapsstr/bytes.A bare
LazyStringserialized as{"sql": "..."}instead of{"sql": ["..."]}, and the client readsObject.values(message)[0][0]; so the toast would have readI.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
1-repro-before-after.mp4
TESTING INSTRUCTIONS
Manual, in SQL Lab against any database:
SELECT ...in a fresh tab and do not run it. The "Save dataset" toolbar button is disabled; hovering it explains why. ("Save query" stays enabled; saving a query needs no results.)SELECT 1 AS asuccessfully, then edit the SQL toSELECT * FROM no_such_tablewithout re-running. "Save dataset" is enabled. Save as new → Save & Explore. The toast readsno such table: no_such_tableand the modal stays open so you can fix and retry.ADDITIONAL INFORMATION