eschutho commented on PR #43081: URL: https://github.com/apache/superset/pull/43081#issuecomment-6208604256
@richardfogaca thanks for the review. I rebased onto master and addressed both points in ea38b8958f: **Async cache-hit path:** `_run_async()` now catches `ChartDataQueryFailedError`, `SemanticLayerQueryRejectedError` and `QueryObjectValidationError` around `command.execute(force_cached=True)`. It returns the same sanitized 400 that `_get_data_response()` returns, and doesn't enqueue a job. When I checked before/after against current master, the behavior was a bit different from what we'd both assumed. `QueryObjectValidationError` has `status = 400`, so on the async path it already returned **400** via the global `SupersetException` handler. But the body was the generic `errors[]` shape and skipped `sanitize_error_message`. The real **500** on that path was `ChartDataQueryFailedError` (a `CommandException`, `status = 500`). The fix and tests cover both. **Tests:** you were right that the test paths in the old description didn't exist. The tests are now: - `tests/unit_tests/charts/test_chart_data_api.py`: `_get_data_response` with `QueryObjectValidationError`, and `_run_async` parametrized over all three exceptions - `tests/integration_tests/charts/data/api_tests.py`: end-to-end sync, plus an async (`GLOBAL_ASYNC_QUERIES=True`, `force=False`) case for `QueryObjectValidationError` and `ChartDataQueryFailedError`. I wrote these as module-level tests on `physical_query_context` because the `TestPostChartDataApi` class is skipped on master and tests added there wouldn't run. With `api.py` reverted, the `_run_async` unit tests and both async integration cases fail. With the fix, they pass. **Rebase note:** the original `_get_data_response()` change landed on master via #45110. After the rebase this PR is just the async path plus tests, so I updated the title and description to match. -- 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]
