eschutho opened a new pull request, #44312: URL: https://github.com/apache/superset/pull/44312
### SUMMARY Fixes a production Sentry crash ([SUPERSET-PYTHON-16YX](https://preset-inc.sentry.io/issues/7733609759/)) — `TypeError: sequence item 0: expected str instance, int found`, surfacing as an unhandled **500** from `ChartDataRestApi.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`, and `SupersetSyntaxErrorException` — re-raising them as a friendly `QueryObjectValidationError` (400). The gap: a `TypeError` raised 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 the `BaseTemplateProcessor.filter_values()` docstring in `superset/jinja_context.py`): ``` WHERE action in ({{ "'" + "','".join(filter_values('action_type')) + "'" }}) ``` `filter_values()` returns `list[Any]` — filter values come from dashboard/native filters and are **not** coerced to `str`. When the filtered column holds numeric values (e.g. an int ID column), `filter_values()` returns `list[int]` and `str.join()` raises `TypeError: sequence item 0: expected str instance, int found` — exactly this issue's message. **Evidence (Sentry traceback, top of stack first):** ``` superset/common/query_actions.py _get_full superset/common/query_context.py get_df_payload superset/common/query_context_processor.py get_df_payload / get_query_result superset/connectors/sqla/models.py query superset/models/helpers.py get_query_str_extended / get_sqla_query superset/connectors/sqla/models.py get_from_clause superset/models/helpers.py get_from_clause -> get_rendered_sql superset/jinja_context.py process_template jinja2/environment.py render <template>:16 top-level template code jinja2/sandbox.py call <-- str.join on list[int] ``` Event count / issue: https://preset-inc.sentry.io/issues/7733609759/ (Sentry issue SUPERSET-PYTHON-16YX). **The fix** Add a `TypeError` except clause in `get_rendered_sql` alongside the existing Jinja guards, wrapping it in `QueryObjectValidationError` using 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 for `get_timestamp_expression`/`convert_tbl_column_to_sqla_col` in the same class (which did not touch `get_rendered_sql`). ### TESTING INSTRUCTIONS Added a regression unit test `test_get_rendered_sql_wraps_type_error` in `tests/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(...)`, assert `QueryObjectValidationError`). Test run: ``` $ pytest tests/unit_tests/connectors/sqla/models_test.py -q 86 passed in 2.82s ``` Lint: ``` $ ruff check superset/models/helpers.py tests/unit_tests/connectors/sqla/models_test.py All checks passed! $ ruff format --check superset/models/helpers.py tests/unit_tests/connectors/sqla/models_test.py 2 files already formatted ``` 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 400 `QueryObjectValidationError`. ### TRADEOFFS - The guard is intentionally scoped to `TypeError` **only**. Other non-Jinja exception types (e.g. `ValueError`, `KeyError`) raised from Python builtins invoked inside user template macros will still surface as 500s. Broadening `get_rendered_sql` to a catch-all `Exception` would 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 - [ ] Has associated issue: - [ ] Required feature flags: - [ ] Changes UI - [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351)) - [ ] Migration is atomic, supports rollback & is backwards-compatible - [ ] Confirm DB migration upgrade and downgrade tested - [ ] Runtime estimates and downtime expectations provided - [ ] Introduces new feature or API - [ ] Removes existing feature or API --- Shortcut: https://app.shortcut.com/preset/story/121034 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
