You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Vitor-Avila (via GitHub)" <gi...@apache.org> on 2026/05/05 22:04:19 UTC
[PR] fix(OAuth2): Support OAuth2 exception with legacy endpoint [superset]
Vitor-Avila opened a new pull request, #39897:
URL: https://github.com/apache/superset/pull/39897
### SUMMARY
This is a follow up to https://github.com/apache/superset/pull/39789, enforcing that the legacy endpoint properly handles the OAuth2 dance.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(OAuth2): Support OAuth2 exception with legacy endpoint [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39897:
URL: https://github.com/apache/superset/pull/39897#issuecomment-4383557773
<!-- Bito Reply -->
The flagged issue is correct. Re-raising SupersetErrorException in get_df_payload breaks async legacy explore jobs, as load_explore_json_into_cache catches generic exceptions and converts them to plain strings, losing structured fields like error_type and OAuth2 extras. To resolve, update load_explore_json_into_cache to handle SupersetErrorException explicitly, similar to load_chart_data_into_cache. Checked PR comments: none found.
**superset/tasks/async_queries.py**
```
import dataclasses
from superset.exceptions import SupersetErrorException
# In load_explore_json_into_cache, modify the except Exception block:
try:
# existing code
except Exception as ex:
if isinstance(ex, SupersetErrorException):
errors = [dataclasses.asdict(ex)]
else:
error = ex.message if hasattr(ex, "message") else str(ex)
errors = [error]
async_query_manager.update_job(..., errors=errors)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(OAuth2): Support OAuth2 exception with legacy endpoint [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #39897:
URL: https://github.com/apache/superset/pull/39897#discussion_r3191881178
##########
superset/viz.py:
##########
@@ -612,6 +613,10 @@ def get_df_payload( # pylint: disable=too-many-statements # noqa: C901
)
self.errors.append(error)
self.status = QueryStatus.FAILED
+ except SupersetErrorException:
+ # Let structured Superset errors (e.g. OAuth2RedirectError) propagate
+ # so the global Flask error handler serializes them.
+ raise
Review Comment:
**Suggestion:** Re-raising `SupersetErrorException` from `get_df_payload` breaks existing caller contracts in async legacy explore jobs: `load_explore_json_into_cache` catches generic exceptions and stores only a plain string error, so structured fields (`error_type`, OAuth2 `extra` with `url/tab_id/redirect_uri`) are lost. That prevents the frontend from triggering the OAuth2 dance for async legacy requests. Keep structured error packaging here for non-request contexts, or update the async explore task to handle `SupersetErrorException` explicitly (like `load_chart_data_into_cache` does) before propagating. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Async legacy /superset/explore_json OAuth2 flow cannot start.
- ⚠️ Users see generic error instead of OAuth2 login prompt.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Enable async legacy explore and OAuth2-backed database:
- GLOBAL_ASYNC_QUERIES feature is enabled so explore uses async jobs (see async branch
in `superset/views/core.py:295-308` `explore_json`).
- Configure a database that can raise `OAuth2RedirectError` / `OAuth2TokenRefreshError`
(subclasses of `SupersetErrorException` defined in `superset/exceptions.py:8-55` and
`334-387`).
2. Trigger an async legacy explore request that will hit the OAuth2 redirect path:
- Call the legacy endpoint `Superset.explore_json` at `superset/views/core.py:295-308`
via `/superset/explore_json/<datasource_type>/<datasource_id>/` with
`response_type=JSON` and GLOBAL_ASYNC_QUERIES enabled.
- The view creates a background job via
`async_query_manager.submit_explore_json_job(...)` at `superset/views/core.py:66-76`,
which in turn enqueues the Celery task `load_explore_json_into_cache`
(`superset/async_events/async_query_manager.py:17-37` and
`superset/tasks/async_queries.py:48-54`).
3. Observe how `SupersetErrorException` now propagates from `get_df_payload` into the
async task:
- In the worker, `load_explore_json_into_cache` builds a `viz_obj` and calls
`viz_obj.get_payload()` at `superset/tasks/async_queries.py:69-77`.
- `viz_obj.get_payload()` in `superset/viz.py:490-530` calls
`self.get_df_payload(query_obj)` at `viz.py:501`.
- Inside `get_df_payload`, the new handler at `superset/viz.py:603-619` now has `except
SupersetErrorException: ... raise`, so an `OAuth2RedirectError` (a
`SupersetErrorException`) thrown during `self.get_df(query_obj)` at `viz.py:79` is
re-raised instead of being converted into `self.errors` / `QueryStatus.FAILED`.
4. See how the async legacy explore job downgrades this structured exception into a plain
string error, losing OAuth2 metadata:
- The re-raised `SupersetErrorException` bubbles out of `viz_obj.get_payload()` back
into `load_explore_json_into_cache`, hitting the generic `except Exception as ex:`
block at `superset/tasks/async_queries.py:15-21` (lines 15–21 in the second snippet
showing that block).
- Because `ex` is a `SupersetErrorException` (not a `SupersetVizException`), the code
takes the `else` branch, computing `error = ex.message if hasattr(ex, "message") else
str(ex)` and `errors = [error]` (plain strings) instead of structured dicts, then calls
`async_query_manager.update_job(..., errors=errors)` at `async_queries.py:22-24`.
- Contrast this with the new chart async path `load_chart_data_into_cache` at
`superset/tasks/async_queries.py:32-41`, which explicitly checks `isinstance(ex,
SupersetErrorException)` and `SupersetErrorsException` and serializes them via
`dataclasses.asdict(...)` into structured error objects that preserve `error_type` and
`extra` (e.g. OAuth2 `url/tab_id/redirect_uri`).
- The async event stream therefore carries `errors` as a list of strings for legacy
explore jobs, not SIP-40-style error dicts with `error_type="OAUTH2_REDIRECT"` and
`extra` required by the frontend component `OAuth2RedirectMessage`
(`superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:34-38,
69-76`), preventing it from starting the OAuth2 dance for async legacy requests.
```
</details>
[Fix in Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fviz.py%0A%2A%2ALine%3A%2A%2A%20616%3A619%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Re-raising%20%60SupersetErrorException%60%20from%20%60get_df_payload%60%20breaks%20existing%20caller%20contracts%20in%20async%20legacy%20explore%20jobs%3A%20%60load_explore_json_into_cache%60%20catches%20generic%20exceptions%20and%20stores%20only%20a%20plain%20string%20error%2C%20so%20structured%20fields%20%28%60error_type%60%2C%20OAuth2%20%60extra%60%20with%20%60url%2Ftab_id%2Fredirect_uri%60%29%20are%20lost.%20That%20prevents%20the%20frontend%20from%20triggering%20the%20OAuth2%20dance%20for%20async%20legacy%20requests.%20Keep%20structured%20error%20packaging%20here%20for%20non-request%20contexts%2C%20or%20update%20the%20async%20explore%20task%20to%20handle%20%60SupersetErrorException%60%20explicitly%20%28like%20%60load_cha
rt_data_into_cache%60%20does%29%20before%20propagating.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A) | [Fix in VSCode Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset%2Fviz.py%0A%2A%2ALine%3A%2A%2A%20616%3A619%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Re-raising%20%60SupersetErrorException%60%20from%20%60get_df_payload%60%20breaks%20existing%20caller%20contracts%20in%20async%
20legacy%20explore%20jobs%3A%20%60load_explore_json_into_cache%60%20catches%20generic%20exceptions%20and%20stores%20only%20a%20plain%20string%20error%2C%20so%20structured%20fields%20%28%60error_type%60%2C%20OAuth2%20%60extra%60%20with%20%60url%2Ftab_id%2Fredirect_uri%60%29%20are%20lost.%20That%20prevents%20the%20frontend%20from%20triggering%20the%20OAuth2%20dance%20for%20async%20legacy%20requests.%20Keep%20structured%20error%20packaging%20here%20for%20non-request%20contexts%2C%20or%20update%20the%20async%20explore%20task%20to%20handle%20%60SupersetErrorException%60%20explicitly%20%28like%20%60load_chart_data_into_cache%60%20does%29%20before%20propagating.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%2
0user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/viz.py
**Line:** 616:619
**Comment:**
*Logic Error: Re-raising `SupersetErrorException` from `get_df_payload` breaks existing caller contracts in async legacy explore jobs: `load_explore_json_into_cache` catches generic exceptions and stores only a plain string error, so structured fields (`error_type`, OAuth2 `extra` with `url/tab_id/redirect_uri`) are lost. That prevents the frontend from triggering the OAuth2 dance for async legacy requests. Keep structured error packaging here for non-request contexts, or update the async explore task to handle `SupersetErrorException` explicitly (like `load_chart_data_into_cache` does) before propagating.
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
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39897&comment_hash=2aad368fab2ed231b4298b4e69a6f830450a004c2dacde38d680c4ad31ae939b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39897&comment_hash=2aad368fab2ed231b4298b4e69a6f830450a004c2dacde38d680c4ad31ae939b&reaction=dislike'>👎</a>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(OAuth2): Support OAuth2 exception with legacy endpoint [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39897:
URL: https://github.com/apache/superset/pull/39897#issuecomment-4383488779
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(OAuth2): Support OAuth2 exception with legacy endpoint [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39897:
URL: https://github.com/apache/superset/pull/39897#issuecomment-4384316585
**Bito Automatic Review Skipped – PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(OAuth2): Support OAuth2 exception with legacy endpoint [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39897:
URL: https://github.com/apache/superset/pull/39897#issuecomment-4383579681
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39897?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `71.42857%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 63.31%. Comparing base ([`9459bc7`](https://app.codecov.io/gh/apache/superset/commit/9459bc7bf4bef8622ad342c9e7d50c6e4469e6ae?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`688574a`](https://app.codecov.io/gh/apache/superset/commit/688574ade21b70148bf7f3328f08a241851d0c77?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/39897?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/views/core.py](https://app.codecov.io/gh/apache/superset/pull/39897?src=pr&el=tree&filepath=superset%2Fviews%2Fcore.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | 60.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39897?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39897 +/- ##
==========================================
- Coverage 63.87% 63.31% -0.56%
==========================================
Files 2582 2582
Lines 136418 136424 +6
Branches 31454 31454
==========================================
- Hits 87137 86378 -759
- Misses 47768 48529 +761
- Partials 1513 1517 +4
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39897/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/39897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `39.29% <14.28%> (-0.01%)` | :arrow_down: |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/39897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.09% <71.42%> (+0.01%)` | :arrow_up: |
| [presto](https://app.codecov.io/gh/apache/superset/pull/39897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `40.99% <14.28%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `59.30% <71.42%> (-1.23%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `58.73% <71.42%> (+0.01%)` | :arrow_up: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39897/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/39897?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(OAuth2): Support OAuth2 exception with legacy endpoint [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #39897:
URL: https://github.com/apache/superset/pull/39897
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org