fix(chart/data): handle QueryObjectValidationError in _get_data_response - #43081
fix(chart/data): handle QueryObjectValidationError in _get_data_response#43081eschutho wants to merge 1 commit into
Conversation
Code Review Agent Run #495cddActionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| except QueryObjectValidationError as exc: | ||
| return self.response_400(message=sanitize_error_message(exc.message)) |
There was a problem hiding this comment.
Suggestion: The new handling only covers _get_data_response, but the GLOBAL_ASYNC_QUERIES path calls command.execute(force_cached=True) directly in _run_async, where only ChartDataCacheLoadError is caught. If execution-time validation in get_payload_result() raises QueryObjectValidationError during the cache lookup, it escapes to Flask and returns HTTP 500 instead of 400. Add equivalent handling to the async cache-hit path before scheduling the background job. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Async chart requests can return HTTP 500 for invalid queries.
- ⚠️ Clients receive server errors instead of validation feedback.
- ⚠️ GLOBAL_ASYNC_QUERIES cache-hit handling remains inconsistent.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/charts/data/api.py
**Line:** 649:650
**Comment:**
*Incomplete Implementation: The new handling only covers `_get_data_response`, but the `GLOBAL_ASYNC_QUERIES` path calls `command.execute(force_cached=True)` directly in `_run_async`, where only `ChartDataCacheLoadError` is caught. If execution-time validation in `get_payload_result()` raises `QueryObjectValidationError` during the cache lookup, it escapes to Flask and returns HTTP 500 instead of 400. Add equivalent handling to the async cache-hit path before scheduling the background job.
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 superset/charts/data/api.py |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43081 +/- ##
==========================================
- Coverage 66.77% 66.62% -0.15%
==========================================
Files 2862 2866 +4
Lines 161657 162547 +890
Branches 37287 37447 +160
==========================================
+ Hits 107943 108301 +358
- Misses 51669 52155 +486
- Partials 2045 2091 +46
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:
|
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
92c4854 to
45b643a
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #2cd5ccActionable 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
ChartDataRestApi._get_data_response()catchesChartDataCacheLoadErrorandChartDataQueryFailedErrorbut leavesQueryObjectValidationErrorunhandled. When this exception escapescommand.execute()through paths outsideget_df_payload_result()'s try/except — such asensure_totals_available(), the invalid-result-type check inget_query_results_with_timing(), or preparers like_prepare_drill_detail_query()— Flask's global handler returns HTTP 500 instead of 400.QueryObjectValidationErroris already imported in this file and hasstatus = 400. This PR adds the missing except clause, consistent with the equivalent handler in the command-setup block above.Testing
pytest tests/unit_tests/charts/data/test_api.pypytest tests/integration_tests/charts/charts_test.pyTracking: sc-117145