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/04/21 04:20:15 UTC
[PR] fix(db oauth2): Improve OAuth2 flow [superset]
Vitor-Avila opened a new pull request, #39499:
URL: https://github.com/apache/superset/pull/39499
### SUMMARY
This PR performs below improvements to the OAuth2 flow:
1. Add a new exception specifically for token refresh failures. This exception inherits from `OAuth2RedirectError`, so that it also triggers `start_oauth2_dance`.
2. Raise 400/401/403 status codes for token refresh at `base.py`, to avoid having to override this on every DB that supports OAuth.
3. Change the `backoff` configuration for the OAuth token, to avoid longer times when a lock is taken.
4. Flush the session after the token is deleted so that it reflects on future attempts.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI changes.
### TESTING INSTRUCTIONS
Test coverage added.
### 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(db oauth2): Improve OAuth2 flow [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #39499:
URL: https://github.com/apache/superset/pull/39499#issuecomment-4285908510
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/39499?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 `37.50000%` with `5 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 55.37%. Comparing base ([`151d7d7`](https://app.codecov.io/gh/apache/superset/commit/151d7d76da2855a7c6c6c83e670e0f367f62a55e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a839ba2`](https://app.codecov.io/gh/apache/superset/commit/a839ba2b30e321872c5fbf864e6e92bf618038ef?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/39499?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/db\_engine\_specs/base.py](https://app.codecov.io/gh/apache/superset/pull/39499?src=pr&el=tree&filepath=superset%2Fdb_engine_specs%2Fbase.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2Jhc2UucHk=) | 33.33% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/utils/oauth2.py](https://app.codecov.io/gh/apache/superset/pull/39499?src=pr&el=tree&filepath=superset%2Futils%2Foauth2.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdXRpbHMvb2F1dGgyLnB5) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/exceptions.py](https://app.codecov.io/gh/apache/superset/pull/39499?src=pr&el=tree&filepath=superset%2Fexceptions.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvZXhjZXB0aW9ucy5weQ==) | 66.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/39499?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
> :exclamation: There is a different number of reports uploaded between BASE (151d7d7) and HEAD (a839ba2). Click for more details.
>
> <details><summary>HEAD has 20 uploads less than BASE</summary>
>
>| Flag | BASE (151d7d7) | HEAD (a839ba2) |
>|------|------|------|
>|python|20|1|
>|presto|2|1|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #39499 +/- ##
==========================================
- Coverage 64.44% 55.37% -9.08%
==========================================
Files 2560 2560
Lines 133574 133566 -8
Branches 31017 31016 -1
==========================================
- Hits 86082 73959 -12123
- Misses 45999 58954 +12955
+ Partials 1493 653 -840
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/39499/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/39499/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/39499/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/39499/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/39499/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.53% <37.50%> (-0.01%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/39499/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `41.53% <37.50%> (-20.39%)` | :arrow_down: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/39499/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/39499/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/39499?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(db oauth2): Improve OAuth2 flow [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #39499:
URL: https://github.com/apache/superset/pull/39499#discussion_r3115387469
##########
tests/unit_tests/db_engine_specs/test_base.py:
##########
@@ -1143,6 +1116,94 @@ class CustomEngineSpec(BaseEngineSpec):
assert request_body["audience"] == "https://api.example.com"
+@with_config({"DATABASE_OAUTH2_TIMEOUT": timedelta(seconds=30)})
+def test_get_oauth2_fresh_token_success(mocker: MockerFixture) -> None:
+ """
+ Test that get_oauth2_fresh_token returns the token response on success.
+ """
+ from superset.db_engine_specs.base import BaseEngineSpec
+
+ mock_post = mocker.patch("superset.db_engine_specs.base.requests.post")
+ mock_post.return_value.status_code = 200
+ mock_post.return_value.json.return_value = {
+ "access_token": "new-access-token",
+ "expires_in": 3600,
+ }
+
+ config: OAuth2ClientConfig = {
+ "id": "client-id",
+ "secret": "client-secret",
+ "scope": "read write",
+ "redirect_uri": "http://localhost:8088/api/v1/database/oauth2/",
+ "authorization_request_uri": "https://oauth.example.com/authorize",
+ "token_request_uri": "https://oauth.example.com/token",
+ "request_content_type": "json",
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated test config</b></div>
<div id="fix">
The OAuth2ClientConfig dictionary is duplicated across test_get_oauth2_fresh_token_success, test_get_oauth2_fresh_token_raises_on_auth_error, and test_get_oauth2_fresh_token_raises_on_server_error. This creates maintenance risk if the configuration needs updates. Consider extracting it to a constant or fixture for better maintainability.
</div>
</div>
<small><i>Code Review Run #6d3adc</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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(db oauth2): Improve OAuth2 flow [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39499:
URL: https://github.com/apache/superset/pull/39499#issuecomment-4286208087
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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(db oauth2): Improve OAuth2 flow [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila merged PR #39499:
URL: https://github.com/apache/superset/pull/39499
--
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(db oauth2): Improve OAuth2 flow [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 #39499:
URL: https://github.com/apache/superset/pull/39499#discussion_r3115023326
##########
superset/utils/oauth2.py:
##########
@@ -143,14 +143,19 @@ def refresh_oauth2_token(
config,
token.refresh_token,
)
- except db_engine_spec.oauth2_exception:
+ except db_engine_spec.oauth2_exception as ex:
# OAuth token is no longer valid, delete it and start OAuth2 dance
logger.warning(
- "OAuth2 token refresh failed for user=%s db=%s, deleting invalid token",
+ (
+ "OAuth2 token refresh failed for user=%s db=%s,"
+ "deleting token. Error: %s",
+ ),
Review Comment:
**Suggestion:** The log message is passed as a single-element tuple instead of a string, so `%` formatting with arguments fails inside logging and emits a logging error instead of the intended warning. Pass a plain format string as the first argument. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ OAuth2 token refresh failures produce logging errors, not warnings.
- ⚠️ Missing user/database context in logs hinders incident diagnosis.
```
</details>
```suggestion
"OAuth2 token refresh failed for user=%s db=%s,"
"deleting token. Error: %s",
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a database that uses OAuth2 (e.g., Google Sheets) so that
`Database._get_sqla_engine` in `superset/models/core.py:50-121` calls
`get_oauth2_access_token` (`superset/utils/oauth2.py:88-124`) whenever
`self.get_oauth2_config()` returns a config and `g.user.id` is present.
2. Ensure there is a `DatabaseUserOAuth2Tokens` entry for the user/database with an
expired `access_token` and a valid `refresh_token`, so `get_oauth2_access_token` calls
`refresh_oauth2_token(config, database_id, user_id, db_engine_spec, token)`
(`superset/utils/oauth2.py:118-119`), which wraps its work in a `DistributedLock`
(`superset/utils/oauth2.py:134-140`) and then calls
`db_engine_spec.get_oauth2_fresh_token(config, token.refresh_token)` at
`superset/utils/oauth2.py:142-145`.
3. Trigger a refresh HTTP error where the token is no longer valid (for example, the user
revoked consent) so that `BaseEngineSpec.get_oauth2_fresh_token`
(`superset/db_engine_specs/base.py:14-39` in the snippet starting at line 15) receives a
400/401/403 response, hits the branch `if response.status_code in (400, 401, 403): raise
OAuth2TokenRefreshError(response.text)` at `superset/db_engine_specs/base.py:36-37`, and
raises `OAuth2TokenRefreshError`, which is a subclass of `OAuth2RedirectError` defined in
`superset/exceptions.py:12-20`. Since `BaseEngineSpec.oauth2_exception` is
`OAuth2RedirectError` (`superset/db_engine_specs/base.py:33-34`), this exception is caught
by `except db_engine_spec.oauth2_exception as ex:` at `superset/utils/oauth2.py:146-156`.
4. Inside this `except` block, `logger.warning` is called with the first positional
argument being a single-element tuple `("OAuth2 token refresh failed for user=%s db=%s,"
"deleting token. Error: %s",)` at `superset/utils/oauth2.py:149-152`, followed by
`user_id`, `database_id`, and `ex` as interpolation arguments. Python's logging pipeline
will create a `LogRecord` with `msg` set to that tuple; `LogRecord.getMessage` then does
`msg = str(self.msg)` giving `"('OAuth2 token refresh failed for user=%s db=%s,deleting
token. Error: %s',)"`, and attempts `msg % self.args`, which raises `TypeError` because
the format string contains no `%s` placeholders after `str()` and there are three
arguments. Logging catches this internally and emits a "Logging error" traceback instead
of the intended warning message; the rest of the handler (`db.session.delete(token)`,
`db.session.flush()`, re-raising the OAuth2 exception) still runs as exercised by
`test_refresh_oauth2_token_deletes_token_on_oauth2_exception` in
`superset/tests/unit_tests/utils/oauth2_tests.py:14-41,` but operators lose the structured
warning with `user_id`, `database_id`, and the error text.
```
</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%2Futils%2Foauth2.py%0A%2A%2ALine%3A%2A%2A%20149%3A152%0A%2A%2AComment%3A%2A%2A%0A%09%2AType%20Error%3A%20The%20log%20message%20is%20passed%20as%20a%20single-element%20tuple%20instead%20of%20a%20string%2C%20so%20%60%25%60%20formatting%20with%20arguments%20fails%20inside%20logging%20and%20emits%20a%20logging%20error%20instead%20of%20the%20intended%20warning.%20Pass%20a%20plain%20format%20string%20as%20the%20first%20argument.%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%20we
ll.%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%2Futils%2Foauth2.py%0A%2A%2ALine%3A%2A%2A%20149%3A152%0A%2A%2AComment%3A%2A%2A%0A%09%2AType%20Error%3A%20The%20log%20message%20is%20passed%20as%20a%20single-element%20tuple%20instead%20of%20a%20string%2C%20so%20%60%25%60%20formatting%20with%20arguments%20fails%20inside%20logging%20and%20emits%20a%20logging%20error%20instead%20of%20the%20intended%20warning.%20Pass%20a%20plain%20format%20string%20as%20the%20first%20argument.%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%20c
heck%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)
*(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/utils/oauth2.py
**Line:** 149:152
**Comment:**
*Type Error: The log message is passed as a single-element tuple instead of a string, so `%` formatting with arguments fails inside logging and emits a logging error instead of the intended warning. Pass a plain format string as the first argument.
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%2F39499&comment_hash=58ca9ea07997c4b4df83208fe1b26485726c5823568dbc4ed51c388e7b61a919&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39499&comment_hash=58ca9ea07997c4b4df83208fe1b26485726c5823568dbc4ed51c388e7b61a919&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(db oauth2): Improve OAuth2 flow [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 #39499:
URL: https://github.com/apache/superset/pull/39499#discussion_r3115008873
##########
superset/exceptions.py:
##########
@@ -368,6 +368,27 @@ def __init__(self, url: str, tab_id: str, redirect_uri: str):
)
+class OAuth2TokenRefreshError(OAuth2RedirectError):
+ """
+ Raised when an OAuth2 refresh token request fails with a 400/401/403 error.
+ The stored token is no longer valid and the user must re-authenticate.
+
+ Subclasses OAuth2RedirectError so that existing oauth2_exception checks
+ match it automatically, triggering start_oauth2_dance() via check_for_oauth2.
+ """
+
+ def __init__(self, response_text: str) -> None:
+ SupersetErrorException.__init__(
Review Comment:
**Suggestion:** Add a docstring to the newly introduced initializer so the new method complies with the requirement that new functions include docstrings. [custom_rule]
**Severity Level:** Minor ⚠️
```suggestion
"""Initialize a refresh-token error that forces OAuth2 re-authentication."""
```
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new initializer in the final file has no docstring, which violates the stated documentation rule for newly added functions. The improved code adds a concise docstring without changing behavior, and the method body remains syntactically valid and executable.
</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%2Fexceptions.py%0A%2A%2ALine%3A%2A%2A%20381%3A381%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20newly%20introduced%20initializer%20so%20the%20new%20method%20complies%20with%20the%20requirement%20that%20new%20functions%20include%20docstrings.%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%2Fexceptions.py%0A%2A%2ALine%3A%2A%2A%20381%3A381%0A%2A%2AComment%3A%2A%2A%0A%09%2ACustom%20Rule%3A%20Add%20a%20docstring%20to%20the%20newly%20introduced%20initializer%20so%20the%20new%20method%20complies%20with%20the%20requirement%20that%20new%20functions%20include%20docstrings.%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)
*(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/exceptions.py
**Line:** 381:381
**Comment:**
*Custom Rule: Add a docstring to the newly introduced initializer so the new method complies with the requirement that new functions include docstrings.
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%2F39499&comment_hash=11388ce14a668eeb019322f136071aa863f619158376a5b2c9c9bd2651d83246&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39499&comment_hash=11388ce14a668eeb019322f136071aa863f619158376a5b2c9c9bd2651d83246&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(db oauth2): Improve OAuth2 flow [superset]
Posted by "Vitor-Avila (via GitHub)" <gi...@apache.org>.
Vitor-Avila commented on code in PR #39499:
URL: https://github.com/apache/superset/pull/39499#discussion_r3115079050
##########
superset/utils/oauth2.py:
##########
@@ -143,14 +143,19 @@ def refresh_oauth2_token(
config,
token.refresh_token,
)
- except db_engine_spec.oauth2_exception:
+ except db_engine_spec.oauth2_exception as ex:
# OAuth token is no longer valid, delete it and start OAuth2 dance
logger.warning(
- "OAuth2 token refresh failed for user=%s db=%s, deleting invalid token",
+ (
+ "OAuth2 token refresh failed for user=%s db=%s,"
+ "deleting token. Error: %s",
+ ),
Review Comment:
good catch! resolved
--
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(db oauth2): Improve OAuth2 flow [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #39499:
URL: https://github.com/apache/superset/pull/39499#issuecomment-4285897580
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