Repository navigation
fix(jinja): handle UndefinedError from virtual dataset templates - #42366
Conversation
Virtual dataset SQL templates that index into filter_values() (e.g.
filter_values('col')[0]) raise a Jinja UndefinedError when no
dashboard filter is active for that column, since the call returns
an empty list. get_rendered_sql() now catches UndefinedError and
re-raises it as a QueryObjectValidationError with a clear message,
turning an unhandled 500 into a graceful chart-level error.
Co-Authored-By: Claude <noreply@anthropic.com>
Code Review Agent Run #46276bActionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42366 +/- ##
==========================================
+ Coverage 65.25% 65.27% +0.02%
==========================================
Files 2794 2808 +14
Lines 157396 158987 +1591
Branches 35982 36209 +227
==========================================
+ Hits 102706 103782 +1076
- Misses 52713 53162 +449
- Partials 1977 2043 +66
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:
|
…on test The previous assertion only matched the exception's message substring, which the pre-existing generic TemplateError handler already produced (UndefinedError is a TemplateError subclass), so the test passed even without the fix. Match on the new "Virtual dataset template error" prefix instead, so the test actually fails without the fix and passes with it. Co-Authored-By: Claude <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| except UndefinedError as ex: | ||
| # Raised when a template references an undefined value, e.g. | ||
| # indexing into an empty list returned by `filter_values()` | ||
| # when no dashboard filter is active for that column. | ||
| raise QueryObjectValidationError( | ||
| _( | ||
| "Virtual dataset template error: %(msg)s", | ||
| msg=str(ex), | ||
| ) | ||
| ) from ex |
There was a problem hiding this comment.
Suggestion: The new handling only applies when rendering through get_rendered_sql(). Virtual dataset metadata discovery renders the same SQL directly via get_virtual_table_metadata() and catches only SupersetSyntaxErrorException, so an empty filter_values('col')[0] still escapes as an unhandled UndefinedError on metadata-related endpoints. Move this translation into shared template-processing handling or add equivalent handling to the metadata path so both virtual-dataset workflows return the intended validation error. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Virtual-dataset metadata endpoint can return an unhandled server error.
- ⚠️ Dataset column discovery fails for unset template filters.
- ⚠️ Metadata refresh also calls `external_metadata()` at `models.py:1978`.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 2778:2787
**Comment:**
*Incomplete Implementation: The new handling only applies when rendering through `get_rendered_sql()`. Virtual dataset metadata discovery renders the same SQL directly via `get_virtual_table_metadata()` and catches only `SupersetSyntaxErrorException`, so an empty `filter_values('col')[0]` still escapes as an unhandled `UndefinedError` on metadata-related endpoints. Move this translation into shared template-processing handling or add equivalent handling to the metadata path so both virtual-dataset workflows return the intended validation error.
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 fix|
The flagged issue is correct. The current implementation only catches To resolve this, you should move the template processing logic into a shared helper method that handles both Would you like me to fetch all other comments on this PR to validate them and implement a minimal fix for the rest? superset/models/helpers.py |
Code Review Agent Run #d90170Actionable 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 |
SUMMARY
Virtual dataset SQL templates that index into
filter_values()(e.g.filter_values('col')[0]) can raise a JinjaUndefinedErrorwhen no dashboard filter is active for that column. This gives that error a dedicated, more specific validation message instead of the generic template-rendering error message it was previously grouped under.PROBLEM
When a virtual dataset's SQL template does something like:
and there are no active dashboard filters for
col,filter_values('col')returns an empty list, and operating on the resulting undefined value (e.g. concatenating it into a string, a common pattern for quoting filter values in SQL) raisesjinja2.exceptions.UndefinedError(e.g.list object has no element 0) during Jinja rendering.get_rendered_sql()insuperset/models/helpers.pyalready caught this via a broaderexcept (TemplateError, SupersetSyntaxErrorException)clause, sinceUndefinedErroris a subclass ofTemplateError— so it was not surfacing as a raw 500. However, it was reported with the same generic message used for unrelated template syntax errors ("Error while rendering virtual dataset query: ..."), which doesn't make it obvious to the dashboard/chart owner that the root cause is a template referencing a value (like an unset filter) that isn't available.FIX
Added an explicit
except UndefinedErrorbranch inget_rendered_sql(), ordered before the existingexcept (TemplateError, SupersetSyntaxErrorException)branch, that re-raises asQueryObjectValidationErrorwith a dedicated message:"Virtual dataset template error: %(msg)s", using the original exception's message.Note: other call sites of
template_processor.process_template()insuperset/models/helpers.py(e.g._process_sql_expression,get_timestamp_expression,convert_tbl_column_to_sqla_col) are not wrapped in equivalent error handling and could still raiseUndefinedErrorunhandled for adhoc SQL expressions. That's out of scope for this PR, which is narrowly focused on the virtual dataset SQL template path.TESTING INSTRUCTIONS
tests/unit_tests/jinja_context_test.pythat builds a virtual dataset whose SQL template indexes intofilter_values('col')[0]with no active filters, and asserts thatget_rendered_sql()raisesQueryObjectValidationErrorwith the new "Virtual dataset template error" message. Verified the test fails against the pre-fix code (with the old generic message) and passes with the fix.pytest tests/unit_tests/jinja_context_test.py tests/unit_tests/models/helpers_test.pylocally — all tests pass.ruff check,ruff format --check,pylint, andmypyon the changed files.ADDITIONAL INFORMATION