Repository navigation
Conversation
… in models.py Four call sites around template_processor.process_template() in superset/connectors/sqla/models.py only caught SupersetSyntaxErrorException, missing jinja2.exceptions.TemplateError (e.g. UndefinedError raised when indexing into filter_values() with no active filter). This let a raw jinja2 exception propagate as an unhandled 500 instead of surfacing as a QueryObjectValidationError, mirroring the same bug class already fixed for get_rendered_sql() and the two other correct call sites in this file. Widen the except clause and message extraction in TableColumn.get_sqla_col, TableColumn.get_timestamp_expression, SqlMetric.get_sqla_col, and SqlaTable._render_adhoc_expression_for_metadata_lookup to match the existing correct sibling sites (RLS filters and get_fetch_values_predicate).
Code Review Agent Run #d58554Actionable 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 |
✅ 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 #42401 +/- ##
==========================================
- Coverage 65.25% 65.25% -0.01%
==========================================
Files 2794 2795 +1
Lines 157410 157482 +72
Branches 35992 36000 +8
==========================================
+ Hits 102720 102763 +43
- Misses 52713 52742 +29
Partials 1977 1977
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:
|
Standing daily pipeline (9:30am PT) that finds one place raising a raw system error instead of a Superset error, fixes it, and ships it through the same PR/Minerva review flow as the Sentry burndown and deprecation-fix jobs. Follows PR apache/superset#42401's manual pattern.
rusackas
left a comment
There was a problem hiding this comment.
Clean fix, widening the except to catch TemplateError matches the two sibling call sites in this same file that already handle it correctly, and the new test pins the exact failure mode (undefined var in a Jinja arithmetic expression). Codecov's 0% patch number looks like a staleness artifact given the report's 1570 commits behind master, not a real gap. Approving.
SUMMARY
Four call sites in
superset/connectors/sqla/models.pythat render Jinja expressions only caughtSupersetSyntaxErrorException, missingjinja2.exceptions.TemplateError. A rawTemplateError(e.g.UndefinedError) from these sites propagated as an unhandled 500 instead of a clear, actionableQueryObjectValidationError. This mirrors the same bug class fixed forget_rendered_sql()in #42366, and this file already has two sibling call sites (RLS filters,get_fetch_values_predicate) that catch the error correctly — this PR brings the remaining four in line with that existing pattern.PROBLEM
TableColumn.get_sqla_col,TableColumn.get_timestamp_expression,SqlMetric.get_sqla_col, andSqlaTable._render_adhoc_expression_for_metadata_lookupeach wraptemplate_processor.process_template(...)in atry/except SupersetSyntaxErrorException. Sincejinja2.exceptions.UndefinedErroris a subclass ofTemplateError, notSupersetSyntaxErrorException, a column/metric/adhoc-column Jinja expression that references an undefined variable (e.g. an arithmetic operation on an undefined name) raises a rawUndefinedErrorthat theseexceptclauses don't catch, surfacing as an unhandled 500 during chart explore/render.FIX
Widen the
exceptclause at all four sites toexcept (TemplateError, SupersetSyntaxErrorException) as ex:and extract the message viagetattr(ex, "message", str(ex)), exactly matching the two already-correct sibling call sites in this same file.TemplateErrorwas already imported at the top of the file. No other exception handling was touched, andsuperset/models/helpers.py(already fixed in #42366) was left untouched.TESTING INSTRUCTIONS
tests/unit_tests/connectors/sqla/models_test.py::test_get_sqla_col_wraps_raw_jinja_undefined_error, which constructs aTableColumnwith the Jinja expression{{ nonexistent_var + 1 }}and assertsget_sqla_col()raisesQueryObjectValidationErrorinstead of a rawUndefinedError. Verified the test fails on pre-fix code and passes after.pytest tests/unit_tests/connectors/sqla/models_test.py(44 passed) andpytest tests/unit_tests/jinja_context_test.py(101 passed, no regressions).ruff checkandmypyon the changed files — no new issues.ADDITIONAL INFORMATION