eschutho commented on code in PR #42851:
URL: https://github.com/apache/superset/pull/42851#discussion_r3848831174
##########
superset/sql/execution/executor.py:
##########
@@ -751,14 +753,18 @@ def _render_sql_template(
:param sql: SQL string potentially containing Jinja2 templates
:param template_params: Parameters to pass to the template
:returns: Rendered SQL string
+ :raises SupersetTemplateException: if the template fails to render
"""
if template_params is None:
return sql
from superset.jinja_context import get_template_processor
tp = get_template_processor(database=self.database)
- return tp.process_template(sql, **template_params)
+ try:
+ return tp.process_template(sql, **template_params)
+ except TemplateError as ex:
+ raise SupersetTemplateException(str(ex)) from ex
Review 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.
--
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]
