eschutho opened a new pull request, #42851:
URL: https://github.com/apache/superset/pull/42851
### SUMMARY
`SQLExecutor._render_sql_template()` (`superset/sql/execution/executor.py`)
called `tp.process_template()` with no `try/except` at all.
`process_template()` can leak a raw `jinja2.exceptions.UndefinedError` for a
template referencing an undefined variable that isn't called as a function.
`execute()` happens to be safe because its whole body is wrapped in a broad
`except Exception`, but `execute_async()` has no such guard — the raw exception
propagated straight out of the public `Database.execute_async()` API contract.
### PROBLEM
`superset/jinja_context.py::BaseTemplateProcessor.process_template()`
converts most Jinja rendering failures into typed exceptions, but has a
bare-`raise` fallback for `UndefinedError` when the undefined variable is
accessed via attribute/subscript syntax rather than a function call. This is
the same gap already fixed at other `process_template()` call sites in this
codebase: #42366, #42401, #42714, #42757, #42802 — this PR fixes it at a new
call site.
`_render_sql_template()` is shared by both `SQLExecutor.execute()` and
`SQLExecutor.execute_async()`. `execute()`'s entire body (including the
`_prepare_sql()` call that reaches `_render_sql_template()`) is wrapped in
`try: ... except Exception as ex: return self._create_error_result(...)`, so
any exception raised there already degrades gracefully into a
`QueryResult(status=FAILED)` — not a bug. `execute_async()` has no equivalent
guard around its `_prepare_sql()` call; confirmed empirically that
`database.execute_async("SELECT {{ missing_var[0] }}",
options=QueryOptions(template_params={"foo": "bar"}))` raised a bare
`jinja2.exceptions.UndefinedError` on `master`, not any `SupersetException`.
This also breaks the method's own established contract: `execute_async()`
already raises typed `SupersetSecurityException` for other prep-time failures
(e.g. disallowed DML), so callers reasonably expect prep-time errors to always
be Superset exceptions.
### FIX
Wrapped the `process_template()` call inside `_render_sql_template()` in
`try/except TemplateError as ex: raise SupersetTemplateException(str(ex)) from
ex`. Reused the existing `SupersetTemplateException` (status 422) rather than
inventing a new exception class — it's already the established general-purpose
Superset exception for Jinja template rendering failures elsewhere in this
codebase (`jinja_context.py` itself raises it for `RecursionError`, and it's
caught in `superset/datasets/api.py` and
`superset/commands/database/validate_sql.py`).
Additive-only: no behavior change to the sync `execute()` path (its broad
`except Exception` still catches the now-typed exception and returns the same
`QueryResult(FAILED)` shape — only the error message text improves).
### TESTING INSTRUCTIONS
Added two tests to `tests/unit_tests/sql/execution/test_executor.py`:
-
`test_execute_async_undefined_template_var_raises_superset_template_exception`
— asserts `execute_async()` with a template referencing an undefined variable
raises `SupersetTemplateException`, not a raw
`jinja2.exceptions.UndefinedError`.
- `test_execute_sync_undefined_template_var_returns_failed_result` — guard
that the sync `execute()` path's error-handling contract (returns
`QueryResult(status=FAILED)`) is unchanged.
- Confirmed the regression test fails on pre-fix code: checked out the
pre-fix version of `executor.py` (test kept), reran — the async test fails with
the raw `jinja2.exceptions.UndefinedError` escaping uncaught.
- Post-fix: full `tests/unit_tests/sql/execution/test_executor.py` passes
(82/82).
- `ruff check` / `ruff format --check`: pass on both changed files.
### ADDITIONAL INFORMATION
- [x] Has associated tests
- [x] Confirmed the regression test fails on pre-fix code and passes post-fix
**Tradeoffs**: none — additive-only exception-handling fix, no behavior
change to any currently-working path.
Related: #42366, #42401, #42714, #42757, #42802 (same `process_template()`
bare-raise-fallback bug class, different call sites).
--
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]
