Repository navigation
Conversation
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 fixThere was a problem hiding this comment.
Thanks for the flag, but I don't think this one holds — the premise is that Spark/Trino are "direct-rendering processors" distinct from the standard one, but they aren't. PrestoTemplateProcessor, HiveTemplateProcessor, SparkTemplateProcessor, and TrinoTemplateProcessor (superset/jinja_context.py:1129–1216) all inherit BaseTemplateProcessor.process_template (:986) unchanged — none override it. That base method already maps compile-time TemplateSyntaxError to a structured SupersetSyntaxErrorException (:1034) for every engine, Spark/Trino included.
Crucially, SupersetSyntaxErrorException subclasses SupersetErrorsException, not jinja2.TemplateError — so the new except TemplateError in _render_sql_template cannot catch or flatten it. The structured syntax-error mapping is fully preserved; nothing is discarded here.
What the new clause actually catches is a raw render-time TemplateError that escapes process_template (the render block at :1057 only special-cases RecursionError/UndefinedError). Before this PR that propagated as an unhandled 500; now it's a contained SupersetTemplateException. So this path is a strict improvement, not a regression.
There is a narrower, legitimate point nearby: render-time template errors don't get the same structured line-metadata mapping that compile-time syntax errors do — they land as a bare SupersetTemplateException. But that's a pre-existing inconsistency independent of this PR (which only contains a previously-unhandled leak), so I'd rather handle it as a separate follow-up than widen this change. Tracking it for a later pass.
|
The review suggestion is correct. The current implementation in superset/sql/execution/executor.py |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42851 +/- ##
==========================================
+ Coverage 82.22% 82.46% +0.24%
==========================================
Files 2995 2995
Lines 185015 188645 +3630
Branches 42818 43620 +802
==========================================
+ Hits 152130 155568 +3438
- Misses 30126 30245 +119
- Partials 2759 2832 +73
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:
|
…or leak in SQLExecutor.execute_async, PR apache/superset#42851
github-actions
Bot
added
the
requires:rebase
4a24fb5 to
176e08c
Compare
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #3ac5e4Actionable 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 |
github-actions
Bot
added
the
requires:rebase
…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.
176e08c to
5617509
Compare
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #6c4b06Actionable Suggestions - 0Additional Suggestions - 2
Review 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 |
This branch has not been deployed
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).