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: [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]
