eschutho opened a new pull request, #42643: URL: https://github.com/apache/superset/pull/42643
### SUMMARY Fixes [SUPERSET-PYTHON-12EZ](https://preset-inc.sentry.io/issues/SUPERSET-PYTHON-12EZ) — `QueryObjectValidationError: Error while rendering virtual dataset query: list object has no element 0`, culprit `ChartDataRestApi.data`, **235,727 events** (the highest-volume unresolved Sentry issue in our production org), 0 users impacted. ### ROOT CAUSE A customer virtual dataset SQL template does something like `{{ filter_values('col')[0] }}`. When no dashboard filter is active for that column, `filter_values()` returns `[]` and Jinja raises `UndefinedError: list object has no element 0` during template rendering. That's already correctly caught in `get_rendered_sql()` (`superset/models/helpers.py`) and re-raised as `QueryObjectValidationError`, a `SupersetException` subclass with `status = 400` — this part of the flow is correct and untouched by this PR. The actual bug: `ChartDataRestApi.data()` (`superset/charts/data/api.py`) is **not** decorated with `@handle_api_exception`, so this well-formed, correctly-classified validation error propagates uncaught all the way to Flask's global catch-all handler (`show_unexpected_exception` in `superset/views/error_handling.py`). That handler unconditionally calls `logger.exception(ex)` (always ERROR + full traceback) and returns HTTP 500 via `json_error_response`'s default status — regardless of the exception's actual `status` attribute. This isn't unique to this one exception type: any `SupersetException` subclass that lacks its own specific `@app.errorhandler` (most of the validation-error types in `superset/exceptions.py` — `QueryObjectValidationError`, `AdvancedDataTypeResponseError`, `InvalidPostProcessingError`, `CacheLoadError`, `NoDataException`, `NullValueException`, `SupersetTemplateException`, `DatabaseNotFound`, `MissingUserContextException`, `QueryClauseValidationException`, `ScreenshotImageNotAvailableException`, etc.) hits this same catch-all whenever it isn't caught by a more specific view-level handler first. Note: there's an unrelated in-flight PR (#42366) touching the same `get_rendered_sql()` function, but for a different (and, on inspection, redundant — `jinja2.exceptions.UndefinedError` is already a subclass of the `TemplateError` the existing `except` clause catches) fix. This PR doesn't touch that function and doesn't conflict with it. ### FIX Added `@app.errorhandler(SupersetException)` in `set_app_error_handlers()`, mirroring the existing (correct) per-view `handle_api_exception` pattern: ```python @app.errorhandler(SupersetException) def show_superset_exception(ex: SupersetException) -> FlaskResponse: logger_func, _ = get_logger_from_status(ex.status) logger_func(ex.message, exc_info=True) return json_error_response( [SupersetError(message=ex.message, error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, level=get_error_level_from_status(ex.status))], status=ex.status, ) ``` `get_logger_from_status` maps 4xx → `logger.warning`, 5xx → `logger.exception` — the same mapping `handle_api_exception` already uses for decorated views. Subclasses with their own specific handler (`SupersetErrorException`, `SupersetErrorsException`, `CommandException`) are unaffected — Flask/Werkzeug dispatches by MRO distance, so those keep routing to their own handlers. ### TRADEOFFS This is a genuine behavior change, disclosed explicitly: for `SupersetException` subclasses with a non-default (non-500) `status` that previously reached this catch-all uncaught, **HTTP status now reflects the real status** (e.g. 400 instead of 500) **and the log level drops from ERROR to WARNING** for 4xx cases. This is more correct, but any API client currently branching on `status == 500` for one of these previously-uncaught validation errors will see a different status code now. Subclasses that don't override `status` (default 500) are unaffected — still logged at ERROR and returned as 500 (covered by a regression test). ### TESTING INSTRUCTIONS - `tests/unit_tests/views/test_error_handling.py` — new `TestShowSupersetException` class: - `test_4xx_superset_exception_returns_its_status_and_logs_at_warning` - `test_5xx_superset_exception_still_returns_500_and_logs_at_error` (no-regression case) - Full `tests/unit_tests/views/` suite (183 tests), `tests/unit_tests/jinja_context_test.py` + `tests/unit_tests/models/helpers_test.py` (248 tests, confirms no overlap with #42366's area): all green. - `ruff check` / `ruff format --check`: clean. Shortcut: https://app.shortcut.com/preset/story/115609 ### 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 -- 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]
