EnxDev commented on code in PR #42472:
URL: https://github.com/apache/superset/pull/42472#discussion_r3673838965
##########
scripts/change_detector.py:
##########
@@ -69,6 +72,35 @@
GITHUB_TOKEN = os.environ.get("GITHUB_TOKEN")
+def _is_rate_limited(err: HTTPError) -> bool:
+ """Whether a 403 is GitHub throttling rather than a missing token scope."""
+ headers = getattr(err, "headers", None)
+ if headers is None:
+ return False
+ try:
+ return (
+ headers.get("x-ratelimit-remaining") == "0"
+ or headers.get("retry-after") is not None
+ )
+ except AttributeError:
+ return False
+
+
+def _api_error_detail(err: object) -> str:
+ """The API's own explanation, e.g. ``Resource not accessible by
integration``.
+
+ Without this a 403 is indistinguishable from a rate limit in the CI log.
+ """
+ try:
+ body = err.read().decode("utf-8", "replace") # type:
ignore[attr-defined]
+ except Exception: # noqa: BLE001 # pylint: disable=broad-except
+ return ""
+ try:
+ return json.loads(body).get("message") or body
+ except (ValueError, AttributeError):
+ return body
Review Comment:
Done. _api_error_detail now bounds its output — return detail[:500] — so a
large or HTML error page can't flood the CI log, while the actionable message
(e.g. "Resource not accessible by integration") is still preserved intact.
##########
tests/unit_tests/subjects/test_utils.py:
##########
@@ -143,6 +146,187 @@ def
test_get_or_create_role_subject_missing_role_returns_none() -> None:
mock_sync.assert_not_called()
+# --------------------------------------------------------------------------
+# get_or_create_group_subject / get_user_group_subjects backfill
+# --------------------------------------------------------------------------
+
+
+@patch("superset.subjects.utils.get_group_subject")
+def test_get_or_create_group_subject_returns_existing(
+ mock_get_group_subject: MagicMock,
+) -> None:
+ """An already-synced group resolves to its Subject with no sync."""
+ existing = _make_subject(10, SubjectType.GROUP)
+ mock_get_group_subject.return_value = existing
+
+ assert get_or_create_group_subject(5) is existing
+ mock_get_group_subject.assert_called_once_with(5)
+
+
+def test_get_or_create_group_subject_syncs_unsynced_group() -> None:
+ """A group that exists but has no Subject row yet is synced on demand
+ rather than treated as nonexistent."""
+ created = _make_subject(11, SubjectType.GROUP)
+ group = MagicMock()
+ group.id = 5
+
+ with (
+ patch(
+ "superset.subjects.utils.get_group_subject",
+ side_effect=[None, created],
+ ) as mock_get_group_subject,
+ patch("superset.subjects.utils.db") as mock_db,
+ patch("superset.subjects.sync.sync_group_subject") as mock_sync,
+ ):
+ mock_db.session.get.return_value = group
+
+ assert get_or_create_group_subject(5) is created
+
+ mock_sync.assert_called_once_with(group)
+ # The insert is wrapped in a SAVEPOINT so a concurrent conflict can't
+ # poison the surrounding transaction.
+ mock_db.session.begin_nested.assert_called_once()
+ assert mock_get_group_subject.call_count == 2
+
+
+def test_get_or_create_group_subject_handles_concurrent_insert() -> None:
+ """A concurrent backfill (IntegrityError in the savepoint) is swallowed and
+ the row the other request wrote is reloaded, keeping the transaction
usable."""
+ from sqlalchemy.exc import IntegrityError
+
+ winner = _make_subject(12, SubjectType.GROUP)
+ group = MagicMock()
+ group.id = 7
+
+ with (
+ patch(
+ "superset.subjects.utils.get_group_subject",
+ side_effect=[None, winner],
+ ) as mock_get_group_subject,
+ patch("superset.subjects.utils.db") as mock_db,
+ patch("superset.subjects.sync.sync_group_subject"),
+ ):
+ mock_db.session.get.return_value = group
+ mock_db.session.begin_nested.return_value.__enter__.side_effect = (
+ IntegrityError("duplicate group_id", None, Exception())
+ )
+
+ assert get_or_create_group_subject(7) is winner
+
+ # First lookup misses, the reload after the conflict returns the winner.
+ assert mock_get_group_subject.call_count == 2
+
+
+def test_get_or_create_group_subject_reraises_unrelated_integrity_error() ->
None:
+ """An IntegrityError that left no matching row isn't the expected
unique-key
+ race, so it propagates instead of silently dropping the group."""
+ from sqlalchemy.exc import IntegrityError
+
+ group = MagicMock()
+ group.id = 7
+
+ with (
+ patch("superset.subjects.utils.get_group_subject", return_value=None),
+ patch("superset.subjects.utils.db") as mock_db,
+ patch("superset.subjects.sync.sync_group_subject"),
+ ):
+ mock_db.session.get.return_value = group
+ mock_db.session.begin_nested.return_value.__enter__.side_effect = (
+ IntegrityError("unrelated failure", None, Exception())
Review Comment:
Fixed. Both group-subject race tests now raise the IntegrityError from
sync_group_subject inside the savepoint (with the mocked __exit__ set to not
suppress it), rather than from __enter__. They exercise the real insert →
conflict → handling path and assert sync_group_subject was called — the
concurrent-insert case reloads and returns the winner, and the
unrelated-failure case propagates.
--
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]
