bito-code-review[bot] commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4131480309
##########
superset/utils/oauth2.py:
##########
@@ -268,6 +268,107 @@ def _refresh_oauth2_token_locked( # noqa: C901
return token.access_token
[email protected]_exception(
+ backoff.expo,
+ AcquireDistributedLockFailedException,
+ factor=0.1,
+ base=2,
+ max_tries=8,
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>duplicated backoff config</b></div>
<div id="fix">
The backoff config `factor=0.1, base=2, max_tries=8` is duplicated verbatim
from `get_oauth2_access_token` (lines 92-94). If one retry policy is tuned
(e.g. max_tries raised for a slow provider), the other silently diverges.
Extract a shared named constant used by both decorators.
</div>
</div>
<small><i>Code Review Run #3adf73</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
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected
token
+ races for the non-blocking refresh lock. The losers must retry until the
winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+
db.session.query().populate_existing().filter_by().one_or_none.return_value =
token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not
available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Failing lock-fallback test</b></div>
<div id="fix">
This test fails against the current implementation.
`_refresh_oauth2_token_forced` is decorated with `@backoff.on_exception(...,
raise_on_giveup=False)`, so when `DistributedLock` always raises
`AcquireDistributedLockFailedException`, backoff swallows it and returns None —
the `except` block in `force_refresh_oauth2_token` that calls
`_read_refreshed_access_token` is never reached, so `result` is None, not
"winning-token". The lock-contention fallback is dead code.
</div>
</div>
<small><i>Code Review Run #3adf73</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
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected
token
+ races for the non-blocking refresh lock. The losers must retry until the
winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+
db.session.query().populate_existing().filter_by().one_or_none.return_value =
token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not
available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_returns_none_when_no_one_refreshed(
+ mocker: MockerFixture,
+) -> None:
+ """With the lock held and the stored token unchanged there is nothing to
reuse."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="rejected-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ mocker.MagicMock(),
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result is None
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Test passes for wrong reason</b></div>
<div id="fix">
This test passes but does not test what its name/docstring claim. Because
the backoff decorator on `_refresh_oauth2_token_forced` swallows the lock
exception, `_read_refreshed_access_token` is never called; `result is None`
holds only because `force_refresh_oauth2_token` returns None directly. The
'stored token unchanged -> nothing to reuse' branch is never exercised.
</div>
</div>
<small><i>Code Review Run #3adf73</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
##########
tests/unit_tests/utils/oauth2_tests.py:
##########
@@ -907,3 +910,127 @@ def test_get_oauth2_redirect_uri_raises_on_runtime_error(
)
with pytest.raises(OAuth2Error):
get_oauth2_redirect_uri()
+
+
+def test_force_refresh_waits_out_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A loser of the refresh race reuses the winner's token instead of raising.
+
+ Every chart on a dashboard opening a connection with the same rejected
token
+ races for the non-blocking refresh lock. The losers must retry until the
winner
+ commits, then short circuit on the committed access token.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ lock = mocker.patch("superset.utils.oauth2.DistributedLock")
+ lock.side_effect = [
+ LockAlreadyHeldException("Lock already taken"),
+ mocker.MagicMock(),
+ ]
+ db_engine_spec = mocker.MagicMock()
+ token = mocker.MagicMock(access_token="winning-token") # noqa: S106
+
db.session.query().populate_existing().filter_by().one_or_none.return_value =
token
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ assert lock.call_count == 2
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_reads_committed_token_when_lock_never_frees(
+ mocker: MockerFixture,
+) -> None:
+ """Giving up on the lock still reuses a token another worker committed."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=AcquireDistributedLockFailedException("Lock not
available"),
+ )
+ db_engine_spec = mocker.MagicMock()
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="winning-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ db_engine_spec,
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result == "winning-token"
+ db_engine_spec.get_oauth2_fresh_token.assert_not_called()
+
+
+def test_force_refresh_returns_none_when_no_one_refreshed(
+ mocker: MockerFixture,
+) -> None:
+ """With the lock held and the stored token unchanged there is nothing to
reuse."""
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ db.session.query().filter_by().one_or_none.return_value = mocker.MagicMock(
+ access_token="rejected-token" # noqa: S106
+ )
+
+ result = force_refresh_oauth2_token(
+ DUMMY_OAUTH2_CONFIG,
+ 1,
+ 2,
+ mocker.MagicMock(),
+ rejected_access_token="rejected-token", # noqa: S106
+ )
+
+ assert result is None
+
+
+def test_execute_with_oauth2_retry_survives_lock_contention(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A chart that loses the refresh race renders with the winner's token.
+
+ Without contention tolerance the lock failure is not an OAuth2 error, so it
+ escapes as an opaque `AcquireDistributedLockFailedException` and the chart
fails.
+ """
+ mocker.patch("time.sleep") # avoid backoff delays in tests
+ operation = mocker.Mock(side_effect=[RuntimeError("stale OAuth token"),
"result"])
+ database = mocker.MagicMock()
+ database.id = 1
+ database.is_oauth2_enabled.return_value = True
+ database.db_engine_spec.needs_oauth2.return_value = True
+ database.get_oauth2_config.return_value = DUMMY_OAUTH2_CONFIG
+ mocker.patch("superset.utils.oauth2.g").user.id = 2
+ db = mocker.patch("superset.utils.oauth2.db")
+ mocker.patch("superset.utils.oauth2.Session", return_value=db.session)
+ mocker.patch(
+ "superset.utils.oauth2.DistributedLock",
+ side_effect=LockAlreadyHeldException("Lock already taken"),
+ )
+ # The rejected token is read first, then the winner's committed
replacement.
+ db.session.query().filter_by().one_or_none.side_effect = [
+ mocker.MagicMock(access_token="rejected-token"), # noqa: S106
+ mocker.MagicMock(access_token="winning-token"), # noqa: S106
+ ]
+
+ assert execute_with_oauth2_retry(database, operation) == "result"
+
+ assert operation.call_count == 2
+ database.start_oauth2_dance.assert_not_called()
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Failing retry test</b></div>
<div id="fix">
This test fails: because the lock exception is swallowed by
`@backoff.on_exception(..., raise_on_giveup=False)` on
`_refresh_oauth2_token_forced`, `force_refresh_oauth2_token` returns None, so
`execute_with_oauth2_retry` (oauth2.py:441-443) calls
`database.start_oauth2_dance()` and `assert_not_called()` fails. The intended
"reuse winner's token" path is never exercised.
</div>
</div>
<small><i>Code Review Run #3adf73</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: [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]
