EnxDev opened a new pull request, #43334:
URL: https://github.com/apache/superset/pull/43334
### 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 raise `SupersetGenericDBErrorException`,
which is not a `SQLAlchemyError`, so `on_error` re-raised it untouched,
`DatasetRestApi.post` had no matching `except`, and it reached FAB's `@safe` as
a bare `500 {"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.** `CreateDatasetCommand`
converts metadata-fetch failures into a `DatasetInvalidError` (422) carrying
the engine's own text:
| SQL | Response |
|---|---|
| `SELECT ...` | `422 {"sql": ["Invalid SQL: Error parsing near '.' at line
1:8"]}` |
| `SELECT * FROM no_such_table` | `422 {"sql": ["no such table:
no_such_table"]}` |
| `SELECT no_such_col FROM birth_names` | `422 {"sql": ["no such column:
no_such_col"]}` |
| `DROP TABLE birth_names` | `422 {"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 to `SELECT
* FROM no_such_table` without 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 the
`catch` was 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 with `lazy_gettext`,
and marshmallow only auto-wraps `str`/`bytes`.
A bare `LazyString` serialized as `{"sql": "..."}` instead of `{"sql":
["..."]}`, and the client reads `Object.values(message)[0][0]` ; so the toast
would have read `I`.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
**Reported repro**; `SELECT ...` left in the editor, never run:
| Before | After |
|---|---|
| Save proceeds, then `500 Fatal error` | Button disabled, tooltip: *You
must run the query successfully first* |
<!-- attach 1-repro-before-after.mp4 -->
**SQL edited after a successful run**; the case the UI gate cannot catch:
| Before | After |
|---|---|
| `500 Fatal error` | `422 no such table: no_such_table`, modal stays open |
<!-- attach 2-stale-sql-before-after.mp4 -->
### TESTING INSTRUCTIONS
Manual, in SQL Lab against any database:
1. Type `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.)
2. Run `SELECT 1 AS a` successfully, then edit the SQL to `SELECT * FROM
no_such_table` without re-running. "Save dataset" is enabled. Save as new →
**Save & Explore**. The toast reads `no such table: no_such_table` and the
modal stays open so you can fix and retry.
3. Run a valid query and save it as a dataset; the normal flow is unchanged
and still opens Explore.
4. Confirm no dataset row is created for the failed attempts.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
