Repository navigation
fix(sqla): guard unprotected Jinja template rendering in ExploreMixin - #44172
Conversation
Wrap `process_template` calls in `get_timestamp_expression` and `convert_tbl_column_to_sqla_col` with the same two-clause exception handler that `get_rendered_sql` already uses, so that `UndefinedError` and `TemplateError` surface as `QueryObjectValidationError` instead of escaping as raw jinja2 exceptions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review Agent Run #09dae3Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44172 +/- ##
==========================================
+ Coverage 80.16% 80.27% +0.10%
==========================================
Files 2925 2928 +3
Lines 172563 174081 +1518
Branches 40058 40378 +320
==========================================
+ Hits 138337 139742 +1405
- Misses 31629 31680 +51
- Partials 2597 2659 +62
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:
|
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Request changes: the calculated-column half is a correct, faithful fix, but the time-column half guards a method with no production caller — the real temporal path still leaks raw UndefinedError, and two new tests assert a guarantee that doesn't hold in production.
🔴 High — Time-column guard is dead code; the live path still leaks
get_timestamp_expression is patched on ExploreMixin (helpers.py:4386), but that method has no production caller. TableColumn doesn't subclass ExploreMixin, and all four call sites (helpers.py:4089/4676/4779/5016) invoke get_timestamp_expression on TableColumn objects with kwargs (apply_dataset_offset=, sql_shifted_temporal_labels=) that only exist on TableColumn.get_timestamp_expression (models.py:1260). That method still wraps process_template with except SupersetSyntaxErrorException only (models.py:1296), so a Jinja error in a time-grain'd temporal calculated column (e.g. {{ nonexistent_var.attr }}) still escapes as raw UndefinedError → 500 — exactly the bug this PR claims to fix. The two get_timestamp_expression tests pass only because they bind the mixin method manually via __get__, giving false green coverage. Fix: move the two-clause handler into TableColumn.get_timestamp_expression at models.py:1295 (where the live catch is), and either delete the now-unused ExploreMixin.get_timestamp_expression or retarget the tests at the real dispatch path. (The convert_tbl_column_to_sqla_col half is genuinely correct — it's ExploreMixin-only and called as self.….)
🟡 Medium — SupersetTemplateException / undefined-function templates bypass the wrapping
The new handlers (and the reference they copy) catch UndefinedError and (TemplateError, SupersetSyntaxErrorException), but process_template also raises UndefinedTemplateFunctionException for {{ missing_macro() }} and SupersetTemplateException for recursion/internal errors (jinja_context.py:1053/1060/1069) — neither is caught. So on the working calculated-column path, {{ some_undefined_func() }} still skips the intended "Calculated column template error" wrapping. Impact is softened because SupersetTemplateException is a SupersetException (rendered as a structured error, not a raw 500), so this is a coverage/consistency gap rather than a crash. Fix: add SupersetTemplateException to the caught tuple (except (TemplateError, SupersetSyntaxErrorException, SupersetTemplateException)) with a str(ex) fallback for the message, ideally in get_rendered_sql too.
🟢 Low — Sibling render paths in the same query still unguarded (asymmetry)
Reachable from the same get_sqla_query, these still catch only SupersetSyntaxErrorException (or nothing): SqlMetric.get_sqla_col (models.py:1433), TableColumn.get_sqla_col (models.py:1220, the adhoc-column reference path), and the fully-unguarded adhoc-SQL renders _process_metric_sql_expression (helpers.py:1937) and _process_validated_sql_expression (helpers.py:1971). Net asymmetry: a calculated column selected directly in groupby is now guarded, but the same column referenced as an adhoc column, or a saved metric like SUM({{ foo.bar }}), still leaks. Out of this PR's stated scope, but worth a follow-up (or expanding here) since it's the same bug class.
🟢 Low — template_error tests inject an exception process_template never emits
test_*_wraps_jinja_template_error mock process_template.side_effect = TemplateSyntaxError(...), but process_template always re-wraps parse-time TemplateSyntaxError as SupersetSyntaxErrorException (jinja_context.py:1034) — a raw TemplateSyntaxError never reaches the handler in production. Because TemplateSyntaxError is a TemplateError, the tests only exercise the ex.message branch; the realistic else: str(ex.errors[0].message …) branch is never covered. Fix: set the side effect to a SupersetSyntaxErrorException (built from a SupersetError) to cover the real branch and the .errors[0] indexing.
🟢 Low — Test assertion/setup nits
The four tests match= only the static prefix ("Time column template error", etc.), not the interpolated original message — so a refactor that blanks msg= to a constant would still pass; consider match=r"…template error.*nonexistent_var". Separately, the two convert_* tests copy database/catalog/schema/engine/_validate_stored_expression setup from …rejects_stored_subquery, but process_template raises before any of it runs — dead setup that misleads readers; drop it (or factor the shared scaffolding into a parametrized helper).
🟢 Low — Pattern nits
The new copies drop the reference's else: # SupersetSyntaxErrorException disambiguator (helpers.py:3578); worth restoring since the else silently corresponds to the second tuple member with a different error shape. The two-clause handler now appears three times in one file — if you dedupe, extract only the _template_error_message(ex) dispatch and keep the _()-wrapped raise inline at each site so pybabel extraction still picks up the strings.
…n coverage Address review feedback (rebenitez1802 High/Medium/Low): - Revert dead ExploreMixin.get_timestamp_expression guard; apply the two-clause UndefinedError + (TemplateError, SupersetSyntaxErrorException, SupersetTemplateException) handler to the real TableColumn dispatch path - Add SupersetTemplateException to the caught tuple in convert_tbl_column_to_sqla_col and get_rendered_sql for consistency - Extend the same guard pattern to TableColumn.get_sqla_col and SqlMetric.get_sqla_col (same get_sqla_query call path) - Fix tests to inject SupersetSyntaxErrorException (what process_template actually emits) instead of raw TemplateSyntaxError - Retarget get_timestamp_expression tests from __get__-bound ExploreMixin to real TableColumn via _stored_col helper - Tighten match= regexes to pin interpolated content; drop dead datasource setup from convert_tbl_column tests - Restore else: # SupersetTemplateException disambiguator comments Deliberately deferred: _process_metric_sql_expression / _process_validated_sql_expression (zero exception handling, noqa: C901, broader adhoc-SQL path) and shared _template_error_message helper (pybabel string-extraction risk).
|
@rebenitez1802 thanks for the thorough pass — the High was a real gap (the mixin method I'd patched has no production caller). Pushed High — reverted Medium — added Low (2 of 4) — extended the identical guard to the two sibling Deliberately not done, flagging rather than guessing at scope: the adhoc-SQL sibling paths you flagged ( Full test file (92 tests) green, ruff/format clean, fail-before/pass-after verified individually for all 8 new/retargeted tests. Ready for another look. |
Code Review Agent Run #4ae788Actionable Suggestions - 0Additional Suggestions - 1
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 |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve — the retarget + exception-widening resolves both round-1 blockers; what's left is optional cleanup.
Resolved: the guard now lives on the real dispatch methods (TableColumn.get_timestamp_expression / get_sqla_col, SqlMetric.get_sqla_col), so the live temporal and metric paths no longer leak raw UndefinedError; every handler — including the get_rendered_sql reference — now also catches SupersetTemplateException / UndefinedTemplateFunctionException; the dead ExploreMixin.get_timestamp_expression edit was dropped; and the tests now hit the real path with realistic exception types (SupersetSyntaxErrorException, SupersetTemplateException) and detail-matching assertions. Dispatch verified correct: UndefinedError catches first (it's a TemplateError subclass), SupersetSyntaxErrorException and SupersetTemplateException are disjoint SupersetException siblings, and UndefinedTemplateFunctionException lands in the else.
🟢 Low — Adhoc-SQL expression paths still unguarded (follow-up)
_process_metric_sql_expression (helpers.py:1938) and _process_validated_sql_expression (helpers.py:1972) still call process_template with no Jinja try/except, so an adhoc metric/column/WHERE/HAVING/ORDER BY sqlExpression with bad Jinja still surfaces a raw error. Pre-existing (not a regression from this PR), so fine to defer — but it's the same bug class and the last sibling in ExploreMixin; worth a follow-up.
🟢 Nit — # noqa: C901 is a symptom of 5× handler duplication
The 3-branch isinstance dispatch is now copy-pasted at five sites (get_sqla_col, get_timestamp_expression, SqlMetric.get_sqla_col, convert_tbl_column_to_sqla_col, get_rendered_sql), which pushed get_timestamp_expression over the complexity threshold. Extracting a module-level _jinja_error_message(ex) -> str and calling it at all five would remove the duplication and let you drop the # noqa: C901. Keep the _()-wrapped raise inline at each site so pybabel still extracts the strings.
🟢 Nit — Message-less TemplateError renders "None"
In if isinstance(ex, TemplateError): error_msg = ex.message, a bare TemplateError() has .message is None, so the surfaced message ends in "None". Contrived (needs a custom filter/extension raising base TemplateError with no message) and inherited from the existing reference, so non-blocking. Fix: error_msg = ex.message or str(ex) (ideally in the extracted helper).
2 participants
SUMMARY
Two
process_templatecall sites inExploreMixin—get_timestamp_expression()andconvert_tbl_column_to_sqla_col()— lack exception handling for Jinja template errors. When a user writes a Jinja expression (e.g.{{ filter_values('col')[0] }}or{{ undefined_var.attr }}) in a time column or calculated column, and the template raisesUndefinedErrororTemplateError, the raw jinja2 exception escapes unhandled instead of being surfaced as aQueryObjectValidationError.This is the same bug class fixed by #42366 and proposed in #42401 for
get_rendered_sql(), which already has the correct two-clause exception handler. These two sibling methods in the same class were missed.Root cause:
BaseTemplateProcessor.process_templatewraps parse-timeTemplateSyntaxErrorasSupersetSyntaxErrorException, but during the render phase it only catchesRecursionErrorand a narrow pattern-matchedUndefinedError; any otherUndefinedErrorand any otherTemplateErrorsubtype raised during rendering escape raw. Both methods are called fromget_sqla_query()(the main chart-rendering / SQL Lab query path) with no upstream catch.Fix: Wrap both
process_templatecalls with the same two-clause handler (except UndefinedError→QueryObjectValidationError, thenexcept (TemplateError, SupersetSyntaxErrorException)→QueryObjectValidationError), mirroring the existing pattern inget_rendered_sql(). All needed imports were already present in the file.This is additive exception handling only — no behavior change for the success path.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — backend-only change affecting error handling, no UI changes.
TESTING INSTRUCTIONS
{{ undefined_var.attr }}jinja2.exceptions.UndefinedErrorescapes, producing an opaque 500 errorQueryObjectValidationErrorwith message "Calculated column template error: 'undefined_var' is undefined" — a clean, user-facing errorAutomated regression tests are included covering both
UndefinedErrorandTemplateSyntaxErrorfor both fixed methods.ADDITIONAL INFORMATION