test(async-queries): prove OAuth2RedirectError survives async chart queries - #41839
Conversation
…ueries (#39096) Adds a unit test asserting that an OAuth2RedirectError raised by ChartDataCommand.run inside load_chart_data_into_cache reaches the async query job as a structured SIP-40 error (OAUTH2_REDIRECT with url/tab_id/ redirect_uri extras) instead of a flattened plain message. This is the exact scenario reported in #39096, fixed by the SIP-40 error extraction from #36596 (chart data path) and #39897 (legacy explore_json path). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Code Review Agent Run #2cd37eActionable 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #41839 +/- ##
==========================================
- Coverage 64.78% 64.77% -0.01%
==========================================
Files 2689 2689
Lines 148936 148936
Branches 34365 34365
==========================================
- Hits 96481 96474 -7
- Misses 50690 50694 +4
- Partials 1765 1768 +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:
|
SUMMARY
Test-only PR that proves issue #39096 is already fixed on master, and closes it.
closes #39096
The issue reported that with
GLOBAL_ASYNC_QUERIESenabled, anOAuth2RedirectErrorraised while loading chart data (e.g. Trino + Keycloak OAuth2) was caught by the genericExceptionhandler inload_chart_data_into_cacheand flattened to a plain{"message": "You don't have permission to access the data."}, losing the structured payload (url,tab_id,redirect_uri) the frontend needs to render the OAuth2 banner.That code path has since been fixed:
load_chart_data_into_cache(the exact handler quoted in the issue), so anySupersetErrorException— includingOAuth2RedirectError, which subclasses it — is serialized viadataclasses.asdict(ex.error)with its fullerror_type/level/extrapayload.load_explore_json_into_cachepath and added an OAuth2-specific test there.What was still missing is a regression test for the issue's exact scenario:
OAuth2RedirectErrorraised byChartDataCommand.runinsideload_chart_data_into_cache. The existing chart-path tests only cover a genericSupersetErrorException, and the OAuth2-specific test from #39897 only covers the explore-json path. This PR adds that complementary unit test, which asserts the async job is updated with the structuredOAUTH2_REDIRECTenvelope (including theurl/tab_id/redirect_uriextras) rather than the flattened permission message. The test passes on current master, confirming the reported behavior is resolved and guarding against regression.No production code is changed.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A (test-only change)
TESTING INSTRUCTIONS
All tests in the module pass, including the new
test_load_chart_data_into_cache_preserves_oauth2_redirect_error.ADDITIONAL INFORMATION
🤖 Generated with Claude Code