fix(errors): map uncaught SupersetException status/log-level correctly - #42643
fix(errors): map uncaught SupersetException status/log-level correctly#42643eschutho wants to merge 2 commits into
Conversation
…y (SC-115609) ChartDataRestApi.data() isn't decorated with @handle_api_exception, so a correctly-classified QueryObjectValidationError (e.g. from a virtual dataset's Jinja template indexing into an empty filter_values() list) propagates uncaught to Flask's global catch-all, which always logs at ERROR and returns HTTP 500 regardless of the exception's real status. Add @app.errorhandler(SupersetException) mirroring the existing handle_api_exception pattern: log level and HTTP status now follow ex.status via get_logger_from_status, instead of being hardcoded to ERROR/500. Subclasses with their own specific handler (SupersetErrorException, SupersetErrorsException, CommandException) are unaffected by Flask's MRO dispatch. 500-status SupersetException subclasses keep logging at ERROR. Fixes SUPERSET-PYTHON-12EZ 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
dosubot
Bot
added
the
global:error
Code Review Agent Run #52adf8Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review 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 AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| SupersetError( | ||
| message=ex.message, | ||
| error_type=SupersetErrorType.GENERIC_BACKEND_ERROR, | ||
| level=get_error_level_from_status(ex.status), | ||
| ), |
There was a problem hiding this comment.
Suggestion: Content negotiation is bypassed for every SupersetException. Previously these exceptions reached show_unexpected_exception, which served Superset's 500.html page for non-debug browser requests accepting HTML; this handler always returns JSON, changing browser-facing error responses. Preserve the existing HTML behavior for HTML requests or route only API requests through the JSON response. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ HTML clients receive JSON error payloads.
- ⚠️ Branded 500-page rendering is bypassed.
- ⚠️ Content negotiation differs across exception classes.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/views/error_handling.py
**Line:** 180:184
**Comment:**
*Api Mismatch: Content negotiation is bypassed for every `SupersetException`. Previously these exceptions reached `show_unexpected_exception`, which served Superset's `500.html` page for non-debug browser requests accepting HTML; this handler always returns JSON, changing browser-facing error responses. Preserve the existing HTML behavior for HTML requests or route only API requests through the JSON response.
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 of Here is a concise fix for from flask import request
@app.errorhandler(SupersetException)
def show_superset_exception(ex: SupersetException) -> FlaskResponse:
if request.accept_mimetypes.accept_html and not request.accept_mimetypes.accept_json:
return show_unexpected_exception(ex)
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,
)There are no other comments on this PR to address. superset/views/error_handling.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42643 +/- ##
==========================================
- Coverage 65.44% 65.42% -0.03%
==========================================
Files 2810 2810
Lines 159362 159442 +80
Branches 36372 36383 +11
==========================================
+ Hits 104301 104319 +18
- Misses 53019 53078 +59
- Partials 2042 2045 +3
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:
|
The new show_superset_exception handler always returned JSON, but the uncaught-exception path it replaces (show_unexpected_exception) served the branded 500.html page for HTML-accepting browser requests. Add the same text/html branch used by show_command_errors/show_http_exception so browser clients keep getting the branded error page instead of a raw JSON payload. 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. |
Code Review Agent Run #5e393bActionable 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 AI Code Review powered by |
1 participant
SUMMARY
Fixes SUPERSET-PYTHON-12EZ —
QueryObjectValidationError: Error while rendering virtual dataset query: list object has no element 0, culpritChartDataRestApi.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 an indexed lookup such as
filter_values('col')[0]. When no dashboard filter is active for that column,filter_values()returns[]and Jinja raisesUndefinedError: list object has no element 0during template rendering.That's already correctly caught in
get_rendered_sql()(superset/models/helpers.py) and re-raised asQueryObjectValidationError, aSupersetExceptionsubclass withstatus = 400— this part of the flow is correct and untouched by this PR.The actual bug: this exception leaks through a specific, unprotected call path.
QueryContextProcessor.get_df_payload()(superset/common/query_context_processor.py) callsself.query_cache_key(query_obj)(line 93) before the method's only try/except block (which starts at line 116 and just wrapsget_query_result/main query execution). When a template uses one of the macros matched byExtraCache.regex(current_user_id,current_username,current_user_rls_rules,url_param, etc. —superset/jinja_context.py),query_cache_key()callsget_extra_cache_keys()→get_sqla_query()→get_from_clause()→get_rendered_sql()to compute the cache key, all outside that try/except. So aQueryObjectValidationErrorraised here is never caught byget_df_payload, andChartDataRestApi.data()(superset/charts/data/api.py) isn't decorated with@handle_api_exceptioneither, so it propagates all the way to Flask's global catch-all handler (show_unexpected_exceptioninsuperset/views/error_handling.py). That handler unconditionally callslogger.exception(ex)(always ERROR + full traceback) and returns HTTP 500 viajson_error_response's default status — regardless of the exception's actualstatusattribute.This isn't unique to this one exception type: any
SupersetExceptionsubclass that lacks its own specific@app.errorhandler(most of the validation-error types insuperset/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.UndefinedErroris already a subclass of theTemplateErrorthe existingexceptclause catches) fix. This PR doesn't touch that function and doesn't conflict with it.FIX
Added
@app.errorhandler(SupersetException)inset_app_error_handlers(), mirroring the existing (correct) per-viewhandle_api_exceptionpattern:get_logger_from_statusmaps 4xx →logger.warning, 5xx →logger.exception— the same mappinghandle_api_exceptionalready 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
SupersetExceptionsubclasses with a non-default (non-500)statusthat 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 onstatus == 500for one of these previously-uncaught validation errors will see a different status code now. Subclasses that don't overridestatus(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— newTestShowSupersetExceptionclass:test_4xx_superset_exception_returns_its_status_and_logs_at_warningtest_5xx_superset_exception_still_returns_500_and_logs_at_error(no-regression case)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 fix(jinja): handle UndefinedError from virtual dataset templates #42366's area): all green.ruff check/ruff format --check: clean.Shortcut: https://app.shortcut.com/preset/story/115609
ADDITIONAL INFORMATION