fix(sql-lab): raise SupersetTemplateException instead of raw UndefinedError in SQLExecutor.execute_async - #42851
fix(sql-lab): raise SupersetTemplateException instead of raw UndefinedError in SQLExecutor.execute_async#42851eschutho wants to merge 1 commit into
Conversation
…dError in SQLExecutor.execute_async _render_sql_template() called process_template() with no try/except, so a Jinja UndefinedError for an undefined variable not called as a function would leak raw past execute_async() (execute() already caught it via its broad except Exception). Wrap the call and re-raise as SupersetTemplateException, consistent with how process_template() itself already handles this failure mode elsewhere.
dosubot
Bot
added
global:jinja
Code Review Agent Run #840521Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| try: | ||
| return tp.process_template(sql, **template_params) | ||
| except TemplateError as ex: | ||
| raise SupersetTemplateException(str(ex)) from ex |
There was a problem hiding this comment.
Suggestion: The broad TemplateError catch flattens compilation-time TemplateSyntaxError exceptions raised by direct-rendering processors such as Spark and Trino into a generic SupersetTemplateException, discarding the structured SupersetSyntaxErrorException/SupersetError details that the standard processor provides. Preserve the existing typed syntax-error mapping before applying the fallback wrapper for raw render-time errors. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Spark and Trino template syntax errors lose line metadata.
- ⚠️ API responses lose structured syntax-error classification.
- ⚠️ SQL Lab users receive less actionable template diagnostics.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/sql/execution/executor.py
**Line:** 764:767
**Comment:**
*Api Mismatch: The broad `TemplateError` catch flattens compilation-time `TemplateSyntaxError` exceptions raised by direct-rendering processors such as Spark and Trino into a generic `SupersetTemplateException`, discarding the structured `SupersetSyntaxErrorException`/`SupersetError` details that the standard processor provides. Preserve the existing typed syntax-error mapping before applying the fallback wrapper for raw render-time errors.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
The review suggestion is correct. The current implementation in superset/sql/execution/executor.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42851 +/- ##
==========================================
- Coverage 65.73% 65.73% -0.01%
==========================================
Files 2843 2843
Lines 162659 162670 +11
Branches 37239 37239
==========================================
+ Hits 106927 106930 +3
- Misses 53640 53648 +8
Partials 2092 2092
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:
|
1 participant
SUMMARY
SQLExecutor._render_sql_template()(superset/sql/execution/executor.py) calledtp.process_template()with notry/exceptat all.process_template()can leak a rawjinja2.exceptions.UndefinedErrorfor 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 broadexcept Exception, butexecute_async()has no such guard — the raw exception propagated straight out of the publicDatabase.execute_async()API contract.PROBLEM
superset/jinja_context.py::BaseTemplateProcessor.process_template()converts most Jinja rendering failures into typed exceptions, but has a bare-raisefallback forUndefinedErrorwhen the undefined variable is accessed via attribute/subscript syntax rather than a function call. This is the same gap already fixed at otherprocess_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 bothSQLExecutor.execute()andSQLExecutor.execute_async().execute()'s entire body (including the_prepare_sql()call that reaches_render_sql_template()) is wrapped intry: ... except Exception as ex: return self._create_error_result(...), so any exception raised there already degrades gracefully into aQueryResult(status=FAILED)— not a bug.execute_async()has no equivalent guard around its_prepare_sql()call; confirmed empirically thatdatabase.execute_async("SELECT {{ missing_var[0] }}", options=QueryOptions(template_params={"foo": "bar"}))raised a barejinja2.exceptions.UndefinedErroronmaster, not anySupersetException. This also breaks the method's own established contract:execute_async()already raises typedSupersetSecurityExceptionfor 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()intry/except TemplateError as ex: raise SupersetTemplateException(str(ex)) from ex. Reused the existingSupersetTemplateException(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.pyitself raises it forRecursionError, and it's caught insuperset/datasets/api.pyandsuperset/commands/database/validate_sql.py).Additive-only: no behavior change to the sync
execute()path (its broadexcept Exceptionstill catches the now-typed exception and returns the sameQueryResult(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— assertsexecute_async()with a template referencing an undefined variable raisesSupersetTemplateException, not a rawjinja2.exceptions.UndefinedError.test_execute_sync_undefined_template_var_returns_failed_result— guard that the syncexecute()path's error-handling contract (returnsQueryResult(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 rawjinja2.exceptions.UndefinedErrorescaping uncaught.Post-fix: full
tests/unit_tests/sql/execution/test_executor.pypasses (82/82).ruff check/ruff format --check: pass on both changed files.ADDITIONAL INFORMATION
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).