bito-code-review[bot] commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4140679361
##########
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:
<!-- Bito Reply -->
The suggestion to extract the backoff configuration into a shared named
constant is appropriate. Using a single source of truth for these parameters
ensures consistency across the expired-token and forced-refresh paths,
preventing potential drift if the retry policy is tuned in the future.
##########
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:
<!-- Bito Reply -->
The reviewer's finding is correct regarding the behavior of the backoff
decorator. When `raise_on_giveup=False` is set, the backoff decorator swallows
the `AcquireDistributedLockFailedException` after the maximum number of retries
and returns `None` instead of raising the exception. Consequently, the `except`
block in `force_refresh_oauth2_token` is not triggered, and the fallback logic
to read the committed access token is bypassed.
To address this, the forced-refresh helper should be configured to allow the
exception to propagate after the retries are exhausted, ensuring the `except`
block can handle the lock failure and proceed with the fallback token read.
##########
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:
<!-- Bito Reply -->
The reviewer's concern regarding the test
`test_force_refresh_returns_none_when_no_one_refreshed` is based on a
misunderstanding of the implementation. As you noted,
`force_refresh_oauth2_token` does not use the backoff decorator directly; it
calls `_retry_forced_refresh_until_lock_available` (which lacks the decorator)
and then proceeds to `_read_refreshed_access_token` if the lock acquisition
fails. The test correctly exercises the branch where the lock is held and no
new token is found, confirming that the function returns `None` as expected in
that scenario.
##########
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:
<!-- Bito Reply -->
The reviewer's concern regarding the test failure is incorrect because
`raise_on_giveup=False` is not applied to the forced-refresh helper. As noted,
the helper (now `_retry_forced_refresh_until_lock_available`) allows the lock
exception to propagate to the `except` block in `force_refresh_oauth2_token`,
ensuring `_read_refreshed_access_token` is executed as intended. The updated
tests in 54572820a6 correctly verify this behavior by asserting the expected
number of lock attempts and confirming the fallback token read occurs.
--
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]
