EnxDev commented on code in PR #42472:
URL: https://github.com/apache/superset/pull/42472#discussion_r3673823842
##########
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())
+ )
+
+ with pytest.raises(IntegrityError):
+ get_or_create_group_subject(7)
Review Comment:
Right — my race tests raised from __enter__, so sync_group_subject never
ran. Reworked both to raise the IntegrityError from the insert (sync) inside
the savepoint (with __exit__ not suppressing) and to assert sync was called.
--
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]
