Repository navigation
fix(sqla): wrap TypeError from virtual dataset Jinja rendering as a validation error - #44312
Conversation
Code Review Agent Run #cd5e21Actionable 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 |
|
@rebenitez1802 — assigning you as reviewer here for continuity on the Jinja template-error / raw-exception-leak bug-class you've carried end-to-end (#42366, #42401, and most recently #44172, also in What it is: Note given #44172: the self-review empirically verified (live MRO/dispatch check under an initialized app) that the patched Load flag: you're carrying the highest active-review count on the roster right now — happy to re-route this to another roster reviewer if you'd rather not take it; just say the word. |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Clean, correctly-scoped defensive fix — turns a real production 500 into a 400, tested, no security-boundary impact. Approving; two non-blocking nits below.
This adds an except TypeError clause to ExploreMixin.get_rendered_sql alongside the existing UndefinedError / (TemplateError, SupersetSyntaxErrorException) guards, so a TypeError from a builtin invoked inside a virtual-dataset template (the documented "','".join(filter_values('numeric_col')) pattern, where filter_values() returns list[int]) surfaces as a QueryObjectValidationError (400) instead of an unhandled 500. Clause ordering is safe (TypeError isn't a subclass of the earlier-caught types), and pytest / ruff check / ruff format --check pass locally. Security-wise it's neutral: str(ex) for a TypeError is a generic Python message (no data leak, same exposure as the adjacent TemplateError branch), and converting 500→400 doesn't touch auth or RLS (RLS is applied after get_rendered_sql in get_from_clause).
🟢 Low — Same TypeError→500 still escapes at sibling process_template call sites
The guard only covers the virtual-dataset FROM clause. The identical filter_values()-into-str.join macro used in a calculated column, timestamp expression, metric, or fetch-values predicate still crashes as a raw 500, because those catch narrower sets: TableColumn.get_sqla_col (superset/connectors/sqla/models.py:1221), TableColumn.get_timestamp_expression (:1296) and SqlMetric.get_sqla_col (:1434) catch only SupersetSyntaxErrorException; SqlaTable.get_fetch_values_predicate (:1885) catches (TemplateError, SupersetSyntaxErrorException) but not TypeError. This is pre-existing behavior and an error-quality parity gap (not a correctness or security bug), so scoping this PR to the reported crash is fine. Follow-up idea: centralize template-error→QueryObjectValidationError mapping in one shared helper used by every process_template(user_sql) site, or note in the description that the sibling call sites are out of scope.
🟢 Low — Test is slightly tautological
test_get_rendered_sql_wraps_type_error mocks process_template.side_effect = TypeError(...), so it verifies the wrapper re-raises but not that the real filter_values()→str.join path actually produces the TypeError. It matches the style of the neighboring test_get_fetch_values_predicate_wraps_undefined_error, so it's acceptable as-is. Optionally strengthen it by asserting the message text and exception chaining (excinfo.value.__cause__ is the original TypeError), or add an integration-style case that renders the documented macro against a numeric filter value end-to-end.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44312 +/- ##
===========================================
+ Coverage 57.23% 80.72% +23.49%
===========================================
Files 2944 2944
Lines 176454 176425 -29
Branches 40926 40916 -10
===========================================
+ Hits 100993 142419 +41426
+ Misses 74486 31328 -43158
- Partials 975 2678 +1703
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:
|
…1034)
`ExploreMixin.get_rendered_sql` catches Jinja-specific exceptions
(`UndefinedError`, `TemplateError`, `SupersetSyntaxErrorException`) and
re-raises them as `QueryObjectValidationError` (400). But a `TypeError`
raised by a Python builtin invoked from *within* the sandboxed template
is none of those, so it escapes as an unhandled 500.
This happens when a virtual dataset's custom SQL uses the documented
`filter_values()` pattern, e.g.
`... in ({{ "'" + "','".join(filter_values('action_type')) + "'" }})`,
and the filtered column holds numeric values. `filter_values()` returns
`list[int]`, and `str.join` raises
`TypeError: sequence item 0: expected str instance, int found`.
Add a `TypeError` except clause alongside the existing ones, wrapping it
in `QueryObjectValidationError` with the existing message style. Scoped
to `TypeError` only.
Fixes SUPERSET-PYTHON-16YX
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
13b5b83 to
70150d4
Compare
Code Review Agent Run #24ff80Actionable 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 |
SUMMARY
Fixes a production Sentry crash (SUPERSET-PYTHON-16YX) —
TypeError: sequence item 0: expected str instance, int found, surfacing as an unhandled 500 fromChartDataRestApi.data.Root cause
ExploreMixin.get_rendered_sql(superset/models/helpers.py) renders a virtual dataset's custom SQL through the sandboxed Jinja engine and already catches Jinja-specific exceptions —UndefinedError,TemplateError, andSupersetSyntaxErrorException— re-raising them as a friendlyQueryObjectValidationError(400). The gap: aTypeErrorraised by a Python builtin invoked from within the template is none of those exception types, so it escapes the guard as an unhandled 500.The concrete trigger is the documented
filter_values()usage pattern (from theBaseTemplateProcessor.filter_values()docstring insuperset/jinja_context.py):filter_values()returnslist[Any]— filter values come from dashboard/native filters and are not coerced tostr. When the filtered column holds numeric values (e.g. an int ID column),filter_values()returnslist[int]andstr.join()raisesTypeError: sequence item 0: expected str instance, int found— exactly this issue's message.Evidence (Sentry traceback, top of stack first):
Event count / issue: https://preset-inc.sentry.io/issues/7733609759/ (Sentry issue SUPERSET-PYTHON-16YX).
The fix
Add a
TypeErrorexcept clause inget_rendered_sqlalongside the existing Jinja guards, wrapping it inQueryObjectValidationErrorusing the existing message style ("Error while rendering virtual dataset query: %(msg)s"). This turns the crash into the same friendly 400 that Jinja-syntax errors already produce. This mirrors the guard pattern established by #44172 forget_timestamp_expression/convert_tbl_column_to_sqla_colin the same class (which did not touchget_rendered_sql).TESTING INSTRUCTIONS
Added a regression unit test
test_get_rendered_sql_wraps_type_errorintests/unit_tests/connectors/sqla/models_test.py, matching the existing style there (ExploreMixin.get_rendered_sql.__get__(datasource),template_processor.process_template.side_effect = TypeError(...), assertQueryObjectValidationError).Test run:
Lint:
To reproduce manually: create a virtual dataset whose SQL uses
{{ "','".join(filter_values('some_numeric_col')) }}, apply a dashboard filter on a numeric column, and load the chart. Before this change the request returns a 500; after it returns a 400QueryObjectValidationError.TRADEOFFS
TypeErroronly. Other non-Jinja exception types (e.g.ValueError,KeyError) raised from Python builtins invoked inside user template macros will still surface as 500s. Broadeningget_rendered_sqlto a catch-allExceptionwould be a larger behavioral change (it could mask genuinely unexpected server errors as user-facing validation errors) and is out of scope here — flagged as a possible future follow-up, not fixed in this PR.ADDITIONAL INFORMATION
Shortcut: https://app.shortcut.com/preset/story/121034
🤖 Generated with Claude Code