Repository navigation
fix(sqla): raise QueryObjectValidationError for template errors in adhoc column metadata lookup - #45742
Conversation
…hoc column metadata lookup Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #45742 +/- ##
==========================================
+ Coverage 84.31% 84.33% +0.01%
==========================================
Files 3059 3059
Lines 202225 202267 +42
Branches 47367 47373 +6
==========================================
+ Hits 170516 170574 +58
+ Misses 28451 28435 -16
Partials 3258 3258
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.
LGTM, thanks for the fix. Verified the new test fails on master with a raw UndefinedError and passes here. One non-blocking note inline.
| try: | ||
| return template_processor.process_template(sql_expression) | ||
| except SupersetSyntaxErrorException as ex: | ||
| except (TemplateError, SupersetSyntaxErrorException) as ex: |
There was a problem hiding this comment.
🟡 Medium: same 500 still reachable via adhoc metrics / metric orderby (OK as a follow-up)
This fixes adhoc columns, and adhoc filters whose col is an adhoc SQL column. A {{ foo.bar }} in an adhoc SQL metric, or in an orderby on one, still returns a 500 from /api/v1/chart/data. Two places are affected:
SqlaTable.adhoc_metric_to_sqla(models.py:2349-2357) only catchesSupersetSecurityException._process_metric_orderby_expressioninsuperset/models/helpers.py(~L5110) has no template-error handling.
That code is outside this diff, so I can't attach an applyable suggestion. Something like this in adhoc_metric_to_sqla would close the metric case. It reuses the existing "metric expression" msgid, so .pot won't drift:
if not processed:
try:
expression = self._process_metric_select_expression(
expression=expression,
engine=self.database.backend,
schema=self.schema,
template_processor=template_processor,
)
except SupersetSecurityException as ex:
raise QueryObjectValidationError(ex.message) from ex
except (TemplateError, SupersetSyntaxErrorException) as ex:
raise QueryObjectValidationError(
_(
"Error in jinja expression in metric expression: %(msg)s",
msg=str(ex),
)
) from exThe orderby path in helpers.py would need the same wrapping. Not blocking: happy to see this in this PR or in a follow-up.
…fy mypy Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SUMMARY
Problem.
SqlaTable._render_adhoc_expression_for_metadata_lookuprenders Jinja in an adhoc column'ssqlExpressionbefore the metadata lookup, but only caughtSupersetSyntaxErrorException.BaseTemplateProcessor.process_templatere-raises a rawjinja2.exceptions.UndefinedErrorwhen the undefined name isn't shaped like a function call (e.g.{{ foo.bar }}). That error escapedadhoc_column_to_sqla, was not handled by the chart-data API (which only mapsQueryObjectValidationErrorto a 400), and fell through to the catch-allExceptionhandler as an opaque 500.Fix. Catch
(TemplateError, SupersetSyntaxErrorException)and raiseQueryObjectValidationErrorwith the existing message text (unchanged).UndefinedErrorsubclassesTemplateError. This matches how sibling sites in the same file already behave (theexcept UndefinedError/except (TemplateError, SupersetSyntaxErrorException)handlers). Only this one site is changed.Same bug class as #42366 and #42401.
Behavior change. A template error in an adhoc column expression now returns a 400 with a validation message instead of a raw 500. The catch is additive; no previously handled path changes.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TESTING INSTRUCTIONS
Added
test_adhoc_column_to_sqla_wraps_undefined_error_in_metadata_lookupintests/unit_tests/connectors/sqla/models_test.py. It uses a realJinjaTemplateProcessorand an adhoc column withsqlExpression="{{ foo.bar }}", and assertsQueryObjectValidationError. Before the fix it fails withjinja2.exceptions.UndefinedError: 'foo' is undefined; after, it passes.Manual: POST a chart-data request with an adhoc column whose
sqlExpressionis{{ foo.bar }}; expect a 400 rather than a 500.Notes on verification:
SqlaTable.adhoc_column_to_sqla(which overrides theExploreMixinone) was confirmed by reading the code, not by a live request.tests/unit_tests/connectors/sqla/models_test.pyhas 6 unrelated failures that also occur without this change (cross-catalog table quoting:test_get_sqla_table_*,test_quoted_name_prevents_double_quoting).ADDITIONAL INFORMATION
🤖 Generated with Claude Code