diff options
| -rw-r--r-- | changelog.d/17435.bugfix | 1 | ||||
| -rw-r--r-- | synapse/storage/databases/main/roommember.py | 57 | ||||
| -rw-r--r-- | tests/rest/client/test_sync.py | 21 | ||||
| -rw-r--r-- | tests/storage/test_roommember.py | 403 |
4 files changed, 456 insertions, 26 deletions
diff --git a/changelog.d/17435.bugfix b/changelog.d/17435.bugfix new file mode 100644 index 0000000000..2d06a7c7fc --- /dev/null +++ b/changelog.d/17435.bugfix @@ -0,0 +1 @@ +Order `heroes` by `stream_ordering` as the Matrix specification states (applies to `/sync`). diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 5d2fd08495..f62d9f705d 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -279,8 +279,19 @@ class RoomMemberWorkerStore(EventsWorkerStore, CacheInvalidationWorkerStore): @cached(max_entries=100000) # type: ignore[synapse-@cached-mutable] async def get_room_summary(self, room_id: str) -> Mapping[str, MemberSummary]: - """Get the details of a room roughly suitable for use by the room + """ + Get the details of a room roughly suitable for use by the room summary extension to /sync. Useful when lazy loading room members. + + Returns the total count of members in the room by membership type, and a + truncated list of members (the heroes). This will be the first 6 members of the + room: + - We want 5 heroes plus 1, in case one of them is the + calling user. + - They are ordered by `stream_ordering`, which are joined or + invited. When no joined or invited members are available, this also includes + banned and left users. + Args: room_id: The room ID to query Returns: @@ -308,23 +319,36 @@ class RoomMemberWorkerStore(EventsWorkerStore, CacheInvalidationWorkerStore): for count, membership in txn: res.setdefault(membership, MemberSummary([], count)) - # we order by membership and then fairly arbitrarily by event_id so - # heroes are consistent - # Note, rejected events will have a null membership field, so - # we we manually filter them out. + # Order by membership (joins -> invites -> leave (former insiders) -> + # everything else (outsiders like bans/knocks), then by `stream_ordering` so + # the first members in the room show up first and to make the sort stable + # (consistent heroes). + # + # Note: rejected events will have a null membership field, so we we manually + # filter them out. sql = """ SELECT state_key, membership, event_id FROM current_state_events WHERE type = 'm.room.member' AND room_id = ? AND membership IS NOT NULL ORDER BY - CASE membership WHEN ? THEN 1 WHEN ? THEN 2 ELSE 3 END ASC, - event_id ASC + CASE membership WHEN ? THEN 1 WHEN ? THEN 2 WHEN ? THEN 3 ELSE 4 END ASC, + event_stream_ordering ASC LIMIT ? """ - # 6 is 5 (number of heroes) plus 1, in case one of them is the calling user. - txn.execute(sql, (room_id, Membership.JOIN, Membership.INVITE, 6)) + txn.execute( + sql, + ( + room_id, + # Sort order + Membership.JOIN, + Membership.INVITE, + Membership.LEAVE, + # 6 is 5 (number of heroes) plus 1, in case one of them is the calling user. + 6, + ), + ) for user_id, membership, event_id in txn: summary = res[membership] # we will always have a summary for this membership type at this @@ -1509,10 +1533,19 @@ def extract_heroes_from_room_summary( ) -> List[str]: """Determine the users that represent a room, from the perspective of the `me` user. + This function expects `MemberSummary.members` to already be sorted by + `stream_ordering` like the results from `get_room_summary(...)`. + The rules which say which users we select are specified in the "Room Summary" section of https://spec.matrix.org/v1.4/client-server-api/#get_matrixclientv3sync + + Args: + details: Mapping from membership type to member summary. We expect + `MemberSummary.members` to already be sorted by `stream_ordering`. + me: The user for whom we are determining the heroes for. + Returns a list (possibly empty) of heroes' mxids. """ empty_ms = MemberSummary([], 0) @@ -1527,11 +1560,11 @@ def extract_heroes_from_room_summary( r[0] for r in details.get(Membership.LEAVE, empty_ms).members if r[0] != me ] + [r[0] for r in details.get(Membership.BAN, empty_ms).members if r[0] != me] - # FIXME: order by stream ordering rather than as returned by SQL + # We expect `MemberSummary.members` to already be sorted by `stream_ordering` if joined_user_ids or invited_user_ids: - return sorted(joined_user_ids + invited_user_ids)[0:5] + return (joined_user_ids + invited_user_ids)[0:5] else: - return sorted(gone_user_ids)[0:5] + return gone_user_ids[0:5] @attr.s(slots=True, auto_attribs=True) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index f5d57e689c..a008ee465b 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -2160,18 +2160,15 @@ class SlidingSyncTestCase(unittest.HomeserverTestCase): # Room2 doesn't have a name so we should see `heroes` populated self.assertIsNone(channel.json_body["rooms"][room_id1].get("name")) - # FIXME: Remove this basic assertion and uncomment the better assertion below - # after https://github.com/element-hq/synapse/pull/17435 merges - self.assertEqual(len(channel.json_body["rooms"][room_id1].get("heroes", [])), 5) - # self.assertCountEqual( - # [ - # hero["user_id"] - # for hero in channel.json_body["rooms"][room_id1].get("heroes", []) - # ], - # # Heroes should be the first 5 users in the room (excluding the user - # # themselves, we shouldn't see `user1`) - # [user2_id, user3_id, user4_id, user5_id, user6_id], - # ) + self.assertCountEqual( + [ + hero["user_id"] + for hero in channel.json_body["rooms"][room_id1].get("heroes", []) + ], + # Heroes should be the first 5 users in the room (excluding the user + # themselves, we shouldn't see `user1`) + [user2_id, user3_id, user4_id, user5_id, user6_id], + ) self.assertEqual( channel.json_body["rooms"][room_id1]["joined_count"], 7, diff --git a/tests/storage/test_roommember.py b/tests/storage/test_roommember.py index 882f3bbbdc..418b556108 100644 --- a/tests/storage/test_roommember.py +++ b/tests/storage/test_roommember.py @@ -19,20 +19,28 @@ # [This file includes modifications made by New Vector Limited] # # +import logging from typing import List, Optional, Tuple, cast from twisted.test.proto_helpers import MemoryReactor -from synapse.api.constants import Membership +from synapse.api.constants import EventTypes, JoinRules, Membership +from synapse.api.room_versions import RoomVersions +from synapse.rest import admin from synapse.rest.admin import register_servlets_for_client_rest_resource -from synapse.rest.client import login, room +from synapse.rest.client import knock, login, room from synapse.server import HomeServer +from synapse.storage.databases.main.roommember import extract_heroes_from_room_summary +from synapse.storage.roommember import MemberSummary from synapse.types import UserID, create_requester from synapse.util import Clock from tests import unittest from tests.server import TestHomeServer from tests.test_utils import event_injection +from tests.unittest import skip_unless + +logger = logging.getLogger(__name__) class RoomMemberStoreTestCase(unittest.HomeserverTestCase): @@ -240,6 +248,397 @@ class RoomMemberStoreTestCase(unittest.HomeserverTestCase): ) +class RoomSummaryTestCase(unittest.HomeserverTestCase): + """ + Test `/sync` room summary related logic like `get_room_summary(...)` and + `extract_heroes_from_room_summary(...)` + """ + + servlets = [ + admin.register_servlets, + knock.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.sliding_sync_handler = self.hs.get_sliding_sync_handler() + self.store = self.hs.get_datastores().main + + def _assert_member_summary( + self, + actual_member_summary: MemberSummary, + expected_member_list: List[str], + *, + expected_member_count: Optional[int] = None, + ) -> None: + """ + Assert that the `MemberSummary` object has the expected members. + """ + self.assertListEqual( + [ + user_id + for user_id, _membership_event_id in actual_member_summary.members + ], + expected_member_list, + ) + self.assertEqual( + actual_member_summary.count, + ( + expected_member_count + if expected_member_count is not None + else len(expected_member_list) + ), + ) + + def test_get_room_summary_membership(self) -> None: + """ + Test that `get_room_summary(...)` gets every kind of membership when there + aren't that many members in the room. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + user3_id = self.register_user("user3", "pass") + _user3_tok = self.login(user3_id, "pass") + user4_id = self.register_user("user4", "pass") + user4_tok = self.login(user4_id, "pass") + user5_id = self.register_user("user5", "pass") + user5_tok = self.login(user5_id, "pass") + + # Setup a room (user1 is the creator and is joined to the room) + room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + + # User2 is banned + self.helper.join(room_id, user2_id, tok=user2_tok) + self.helper.ban(room_id, src=user1_id, targ=user2_id, tok=user1_tok) + + # User3 is invited by user1 + self.helper.invite(room_id, targ=user3_id, tok=user1_tok) + + # User4 leaves + self.helper.join(room_id, user4_id, tok=user4_tok) + self.helper.leave(room_id, user4_id, tok=user4_tok) + + # User5 joins + self.helper.join(room_id, user5_id, tok=user5_tok) + + room_membership_summary = self.get_success(self.store.get_room_summary(room_id)) + empty_ms = MemberSummary([], 0) + + self._assert_member_summary( + room_membership_summary.get(Membership.JOIN, empty_ms), + [user1_id, user5_id], + ) + self._assert_member_summary( + room_membership_summary.get(Membership.INVITE, empty_ms), [user3_id] + ) + self._assert_member_summary( + room_membership_summary.get(Membership.LEAVE, empty_ms), [user4_id] + ) + self._assert_member_summary( + room_membership_summary.get(Membership.BAN, empty_ms), [user2_id] + ) + self._assert_member_summary( + room_membership_summary.get(Membership.KNOCK, empty_ms), + [ + # No one knocked + ], + ) + + def test_get_room_summary_membership_order(self) -> None: + """ + Test that `get_room_summary(...)` stacks our limit of 6 in this order: joins -> + invites -> leave -> everything else (bans/knocks) + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + user3_id = self.register_user("user3", "pass") + _user3_tok = self.login(user3_id, "pass") + user4_id = self.register_user("user4", "pass") + user4_tok = self.login(user4_id, "pass") + user5_id = self.register_user("user5", "pass") + user5_tok = self.login(user5_id, "pass") + user6_id = self.register_user("user6", "pass") + user6_tok = self.login(user6_id, "pass") + user7_id = self.register_user("user7", "pass") + user7_tok = self.login(user7_id, "pass") + + # Setup the room (user1 is the creator and is joined to the room) + room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + + # We expect the order to be joins -> invites -> leave -> bans so setup the users + # *NOT* in that same order to make sure we're actually sorting them. + + # User2 is banned + self.helper.join(room_id, user2_id, tok=user2_tok) + self.helper.ban(room_id, src=user1_id, targ=user2_id, tok=user1_tok) + + # User3 is invited by user1 + self.helper.invite(room_id, targ=user3_id, tok=user1_tok) + + # User4 leaves + self.helper.join(room_id, user4_id, tok=user4_tok) + self.helper.leave(room_id, user4_id, tok=user4_tok) + + # User5, User6, User7 joins + self.helper.join(room_id, user5_id, tok=user5_tok) + self.helper.join(room_id, user6_id, tok=user6_tok) + self.helper.join(room_id, user7_id, tok=user7_tok) + + room_membership_summary = self.get_success(self.store.get_room_summary(room_id)) + empty_ms = MemberSummary([], 0) + + self._assert_member_summary( + room_membership_summary.get(Membership.JOIN, empty_ms), + [user1_id, user5_id, user6_id, user7_id], + ) + self._assert_member_summary( + room_membership_summary.get(Membership.INVITE, empty_ms), [user3_id] + ) + self._assert_member_summary( + room_membership_summary.get(Membership.LEAVE, empty_ms), [user4_id] + ) + self._assert_member_summary( + room_membership_summary.get(Membership.BAN, empty_ms), + [ + # The banned user is not in the summary because the summary can only fit + # 6 members and prefers everything else before bans + # + # user2_id + ], + # But we still see the count of banned users + expected_member_count=1, + ) + self._assert_member_summary( + room_membership_summary.get(Membership.KNOCK, empty_ms), + [ + # No one knocked + ], + ) + + def test_extract_heroes_from_room_summary_excludes_self(self) -> None: + """ + Test that `extract_heroes_from_room_summary(...)` does not include the user + itself. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + + # Setup the room (user1 is the creator and is joined to the room) + room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + + # User2 joins + self.helper.join(room_id, user2_id, tok=user2_tok) + + room_membership_summary = self.get_success(self.store.get_room_summary(room_id)) + + # We first ask from the perspective of a random fake user + hero_user_ids = extract_heroes_from_room_summary( + room_membership_summary, me="@fakeuser" + ) + + # Make sure user1 is in the room (ensure our test setup is correct) + self.assertListEqual(hero_user_ids, [user1_id, user2_id]) + + # Now, we ask for the room summary from the perspective of user1 + hero_user_ids = extract_heroes_from_room_summary( + room_membership_summary, me=user1_id + ) + + # User1 should not be included in the list of heroes because they are the one + # asking + self.assertListEqual(hero_user_ids, [user2_id]) + + def test_extract_heroes_from_room_summary_first_five_joins(self) -> None: + """ + Test that `extract_heroes_from_room_summary(...)` returns the first 5 joins. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + user3_id = self.register_user("user3", "pass") + user3_tok = self.login(user3_id, "pass") + user4_id = self.register_user("user4", "pass") + user4_tok = self.login(user4_id, "pass") + user5_id = self.register_user("user5", "pass") + user5_tok = self.login(user5_id, "pass") + user6_id = self.register_user("user6", "pass") + user6_tok = self.login(user6_id, "pass") + user7_id = self.register_user("user7", "pass") + user7_tok = self.login(user7_id, "pass") + + # Setup the room (user1 is the creator and is joined to the room) + room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + + # User2 -> User7 joins + self.helper.join(room_id, user2_id, tok=user2_tok) + self.helper.join(room_id, user3_id, tok=user3_tok) + self.helper.join(room_id, user4_id, tok=user4_tok) + self.helper.join(room_id, user5_id, tok=user5_tok) + self.helper.join(room_id, user6_id, tok=user6_tok) + self.helper.join(room_id, user7_id, tok=user7_tok) + + room_membership_summary = self.get_success(self.store.get_room_summary(room_id)) + + hero_user_ids = extract_heroes_from_room_summary( + room_membership_summary, me="@fakuser" + ) + + # First 5 users to join the room + self.assertListEqual( + hero_user_ids, [user1_id, user2_id, user3_id, user4_id, user5_id] + ) + + def test_extract_heroes_from_room_summary_membership_order(self) -> None: + """ + Test that `extract_heroes_from_room_summary(...)` prefers joins/invites over + everything else. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + user3_id = self.register_user("user3", "pass") + _user3_tok = self.login(user3_id, "pass") + user4_id = self.register_user("user4", "pass") + user4_tok = self.login(user4_id, "pass") + user5_id = self.register_user("user5", "pass") + user5_tok = self.login(user5_id, "pass") + + # Setup the room (user1 is the creator and is joined to the room) + room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + + # We expect the order to be joins -> invites -> leave -> bans so setup the users + # *NOT* in that same order to make sure we're actually sorting them. + + # User2 is banned + self.helper.join(room_id, user2_id, tok=user2_tok) + self.helper.ban(room_id, src=user1_id, targ=user2_id, tok=user1_tok) + + # User3 is invited by user1 + self.helper.invite(room_id, targ=user3_id, tok=user1_tok) + + # User4 leaves + self.helper.join(room_id, user4_id, tok=user4_tok) + self.helper.leave(room_id, user4_id, tok=user4_tok) + + # User5 joins + self.helper.join(room_id, user5_id, tok=user5_tok) + + room_membership_summary = self.get_success(self.store.get_room_summary(room_id)) + + hero_user_ids = extract_heroes_from_room_summary( + room_membership_summary, me="@fakeuser" + ) + + # Prefer joins -> invites, over everything else + self.assertListEqual( + hero_user_ids, + [ + # The joins + user1_id, + user5_id, + # The invites + user3_id, + ], + ) + + @skip_unless( + False, + "Test is not possible because when everyone leaves the room, " + + "the server is `no_longer_in_room` and we don't have any `current_state_events` to query", + ) + def test_extract_heroes_from_room_summary_fallback_leave_ban(self) -> None: + """ + Test that `extract_heroes_from_room_summary(...)` falls back to leave/ban if + there aren't any joins/invites. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + user3_id = self.register_user("user3", "pass") + user3_tok = self.login(user3_id, "pass") + + # Setup the room (user1 is the creator and is joined to the room) + room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + + # User2 is banned + self.helper.join(room_id, user2_id, tok=user2_tok) + self.helper.ban(room_id, src=user1_id, targ=user2_id, tok=user1_tok) + + # User3 leaves + self.helper.join(room_id, user3_id, tok=user3_tok) + self.helper.leave(room_id, user3_id, tok=user3_tok) + + # User1 leaves (we're doing this last because they're the room creator) + self.helper.leave(room_id, user1_id, tok=user1_tok) + + room_membership_summary = self.get_success(self.store.get_room_summary(room_id)) + + hero_user_ids = extract_heroes_from_room_summary( + room_membership_summary, me="@fakeuser" + ) + + # Fallback to people who left -> banned + self.assertListEqual( + hero_user_ids, + [user3_id, user1_id, user3_id], + ) + + def test_extract_heroes_from_room_summary_excludes_knocks(self) -> None: + """ + People who knock on the room have (potentially) never been in the room before + and are total outsiders. Plus the spec doesn't mention them at all for heroes. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + user2_id = self.register_user("user2", "pass") + user2_tok = self.login(user2_id, "pass") + + # Setup the knock room (user1 is the creator and is joined to the room) + knock_room_id = self.helper.create_room_as( + user1_id, tok=user1_tok, room_version=RoomVersions.V7.identifier + ) + self.helper.send_state( + knock_room_id, + EventTypes.JoinRules, + {"join_rule": JoinRules.KNOCK}, + tok=user1_tok, + ) + + # User2 knocks on the room + knock_channel = self.make_request( + "POST", + "/_matrix/client/r0/knock/%s" % (knock_room_id,), + b"{}", + user2_tok, + ) + self.assertEqual(knock_channel.code, 200, knock_channel.result) + + room_membership_summary = self.get_success( + self.store.get_room_summary(knock_room_id) + ) + + hero_user_ids = extract_heroes_from_room_summary( + room_membership_summary, me="@fakeuser" + ) + + # user1 is the creator and is joined to the room (should show up as a hero) + # user2 is knocking on the room (should not show up as a hero) + self.assertListEqual( + hero_user_ids, + [user1_id], + ) + + class CurrentStateMembershipUpdateTestCase(unittest.HomeserverTestCase): def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = hs.get_datastores().main |