Abdulrehman-PIAIC80387 opened a new pull request, #42463:
URL: https://github.com/apache/superset/pull/42463
### SUMMARY
Fixes #38012.
Saving a virtual dataset whose SQL contains Jinja templates (e.g. `{% if
from_dttm %}...{% endif %}`) via the Dataset Editor modal shows an "Invalid
SQL" error toast even though the dataset row is successfully saved.
The error comes from `RefreshDatasetCommand`, which runs after
`UpdateDatasetCommand` when `override_columns=True` and re-parses the Jinja SQL
with no runtime context — `sqlglot` then fails on the empty `{% if %}` blocks
and the whole PUT surfaces as a failure.
Per @rusackas on the issue thread, the post-save column refresh should be
**best-effort**: the row is already committed, so a Jinja-driven parse failure
should not surface as a hard error.
**Fix:** catch `SupersetGenericDBErrorException` inside
`RefreshDatasetCommand.run()`, log a warning naming the dataset, and return the
model. Other exceptions (security, unexpected errors) still propagate as before.
### SCOPE
This is a **pragmatic best-effort** fix, not a full architectural solution.
It stops the misleading error message but does not solve the underlying issue
that `get_virtual_table_metadata()` has no runtime context.
**What this PR does:**
- Post-save column refresh becomes best-effort for the specific
`SupersetGenericDBErrorException` case (Jinja parse failure)
- User no longer sees "Invalid SQL" toast for a save that actually succeeded
- Warning logged for observability
**What this PR does NOT do:**
- The Jinja SQL still cannot be validated at save time without runtime
context
- Column metadata may be stale for Jinja datasets after save (unchanged from
today)
- Other callers of `fetch_metadata()` (import/export, SQL Lab "Create
Dataset from Query") still fail hard on the same code path
**The bigger fix** would propagate `template_params` through
`get_virtual_table_metadata()` and all its callers so validation can render
with real context. That's a much larger diff (function signature changes across
~5 files, backward-compat considerations) and I'd prefer to do it as a
follow-up PR unless you want it here.
**Happy either way:**
- Merge this as-is + I'll open a follow-up issue for the context-propagation
fix
- OR expand this PR to include the propagation — say the word and I'll add
it here
Let me know your preference @rusackas.
### TESTING INSTRUCTIONS
Manual:
1. Create a virtual dataset with SQL containing a Jinja block:
```sql
SELECT * FROM foo {% if from_dttm %}WHERE ds > '{{ from_dttm }}'{% endif
%}
```
2. Save via the Dataset Editor modal
3. **Before:** "Invalid SQL" toast (row saved anyway)
4. **After:** save completes silently; warning in server log
Automated:
```bash
pytest tests/unit_tests/commands/dataset/refresh_test.py -v
```
Four tests: Jinja-parse softening, security exception still propagates,
dataset-not-found still raises, forbidden still raises. All pass locally + no
regressions in the broader `tests/unit_tests/commands/dataset/` suite.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #38012
- [x] Required feature flags: none
- [x] Changes UI: no
- [x] Includes DB Migration: no
- [x] Includes CLI or Node.js commands: no
- [x] Breaking change: no
--
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]
