From 9deb387f24ea33d6121ef10012a1898225516aa3 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 8 Jul 2024 15:35:25 -0500 Subject: [PATCH 01/39] Initial work of adding name/avatar to rooms response --- synapse/handlers/sliding_sync.py | 151 ++++++++++++++++++++----------- tests/rest/client/test_sync.py | 8 ++ 2 files changed, 104 insertions(+), 55 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 8e2f751c02..bb81ca9d97 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -18,6 +18,7 @@ # # import logging +from itertools import chain from typing import TYPE_CHECKING, Any, Dict, Final, List, Optional, Set, Tuple import attr @@ -464,6 +465,7 @@ async def current_sync_for_user( membership_state_keys = room_sync_config.required_state_map.get( EventTypes.Member ) + # Also see `StateFilter.must_await_full_state(...)` for comparison lazy_loading = ( membership_state_keys is not None and len(membership_state_keys) == 1 @@ -1202,7 +1204,7 @@ async def get_room_sync_data( # Figure out any stripped state events for invite/knocks. This allows the # potential joiner to identify the room. - stripped_state: List[JsonDict] = [] + stripped_state: Optional[List[JsonDict]] = None if room_membership_for_user_at_to_token.membership in ( Membership.INVITE, Membership.KNOCK, @@ -1239,7 +1241,7 @@ async def get_room_sync_data( # updates. initial = True - # Fetch the required state for the room + # Fetch the `required_state` for the room # # No `required_state` for invite/knock rooms (just `stripped_state`) # @@ -1247,13 +1249,15 @@ async def get_room_sync_data( # of membership. Currently, we have to make this optional because # `invite`/`knock` rooms only have `stripped_state`. See # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1653045932 + # + # Calculate the `StateFilter` based on the `required_state` for the room room_state: Optional[StateMap[EventBase]] = None + required_room_state: Optional[StateMap[EventBase]] = None if room_membership_for_user_at_to_token.membership not in ( Membership.INVITE, Membership.KNOCK, ): - # Calculate the `StateFilter` based on the `required_state` for the room - state_filter: Optional[StateFilter] = StateFilter.none() + required_state_filter = StateFilter.none() # If we have a double wildcard ("*", "*") in the `required_state`, we need # to fetch all state for the room # @@ -1276,7 +1280,7 @@ async def get_room_sync_data( if StateValues.WILDCARD in room_sync_config.required_state_map.get( StateValues.WILDCARD, set() ): - state_filter = StateFilter.all() + required_state_filter = StateFilter.all() # TODO: `StateFilter` currently doesn't support wildcard event types. We're # currently working around this by returning all state to the client but it # would be nice to fetch less from the database and return just what the @@ -1285,7 +1289,7 @@ async def get_room_sync_data( room_sync_config.required_state_map.get(StateValues.WILDCARD) is not None ): - state_filter = StateFilter.all() + required_state_filter = StateFilter.all() else: required_state_types: List[Tuple[str, Optional[str]]] = [] for ( @@ -1317,51 +1321,88 @@ async def get_room_sync_data( else: required_state_types.append((state_type, state_key)) - state_filter = StateFilter.from_types(required_state_types) - - # We can skip fetching state if we don't need any - if state_filter != StateFilter.none(): - # We can return all of the state that was requested if we're doing an - # initial sync - if initial: - # People shouldn't see past their leave/ban event - if room_membership_for_user_at_to_token.membership in ( - Membership.LEAVE, - Membership.BAN, - ): - room_state = await self.storage_controllers.state.get_state_at( - room_id, - stream_position=to_token.copy_and_replace( - StreamKeyType.ROOM, - room_membership_for_user_at_to_token.event_pos.to_room_stream_token(), - ), - state_filter=state_filter, - # Partially-stated rooms should have all state events except for - # the membership events and since we've already excluded - # partially-stated rooms unless `required_state` only has - # `["m.room.member", "$LAZY"]` for membership, we should be able - # to retrieve everything requested. Plus we don't want to block - # the whole sync waiting for this one room. - await_full_state=False, - ) - # Otherwise, we can get the latest current state in the room - else: - room_state = await self.storage_controllers.state.get_current_state( - room_id, - state_filter, - # Partially-stated rooms should have all state events except for - # the membership events and since we've already excluded - # partially-stated rooms unless `required_state` only has - # `["m.room.member", "$LAZY"]` for membership, we should be able - # to retrieve everything requested. Plus we don't want to block - # the whole sync waiting for this one room. - await_full_state=False, - ) - # TODO: Query `current_state_delta_stream` and reverse/rewind back to the `to_token` + required_state_filter = StateFilter.from_types(required_state_types) + + # We need this base set of info for the response so let's just fetch it along + # with the `required_state` for the room + META_ROOM_STATE = [(EventTypes.Name, ""), (EventTypes.RoomAvatar, "")] + state_filter = StateFilter( + types=StateFilter.from_types( + chain(META_ROOM_STATE, required_state_filter.to_types()) + ).types, + include_others=required_state_filter.include_others, + ) + + # We can return all of the state that was requested if this was the first + # time we've sent the room down this connection. + if initial: + # People shouldn't see past their leave/ban event + if room_membership_for_user_at_to_token.membership in ( + Membership.LEAVE, + Membership.BAN, + ): + room_state = await self.storage_controllers.state.get_state_at( + room_id, + stream_position=to_token.copy_and_replace( + StreamKeyType.ROOM, + room_membership_for_user_at_to_token.event_pos.to_room_stream_token(), + ), + state_filter=state_filter, + # Partially-stated rooms should have all state events except for + # remote membership events. Since we've already excluded + # partially-stated rooms unless `required_state` only has + # `["m.room.member", "$LAZY"]` for membership, we should be able to + # retrieve everything requested. When we're lazy-loading, if there + # are some remote senders in the timeline, we should also have their + # membership event because we had to auth that timeline event. Plus + # we don't want to block the whole sync waiting for this one room. + await_full_state=False, + ) + # Otherwise, we can get the latest current state in the room else: - # TODO: Once we can figure out if we've sent a room down this connection before, - # we can return updates instead of the full required state. - raise NotImplementedError() + room_state = await self.storage_controllers.state.get_current_state( + room_id, + state_filter, + # Partially-stated rooms should have all state events except for + # remote membership events. Since we've already excluded + # partially-stated rooms unless `required_state` only has + # `["m.room.member", "$LAZY"]` for membership, we should be able to + # retrieve everything requested. When we're lazy-loading, if there + # are some remote senders in the timeline, we should also have their + # membership event because we had to auth that timeline event. Plus + # we don't want to block the whole sync waiting for this one room. + await_full_state=False, + ) + # TODO: Query `current_state_delta_stream` and reverse/rewind back to the `to_token` + else: + # TODO: Once we can figure out if we've sent a room down this connection before, + # we can return updates instead of the full required state. + raise NotImplementedError() + + if required_state_filter != StateFilter.none(): + required_room_state = required_state_filter.filter_state(room_state) + + # Find the room name and avatar from the state + room_name: Optional[str] = None + room_avatar: Optional[str] = None + if room_state is not None: + name_event = room_state.get((EventTypes.Name, "")) + if name_event is not None: + room_name = name_event.content.get("name") + + avatar_event = room_state.get((EventTypes.RoomAvatar, "")) + if avatar_event is not None: + room_avatar = avatar_event.content.get("url") + elif stripped_state is not None: + for event in stripped_state: + if event["type"] == EventTypes.Name: + room_name = event.get("content", {}).get("name") + elif event["type"] == EventTypes.RoomAvatar: + room_avatar = event.get("content", {}).get("url") + + # Found everything so we can stop looking + if room_name is not None and room_avatar is not None: + break # Figure out the last bump event in the room last_bump_event_result = ( @@ -1378,16 +1419,16 @@ async def get_room_sync_data( bump_stamp = bump_event_pos.stream return SlidingSyncResult.RoomResult( - # TODO: Dummy value - name=None, - # TODO: Dummy value - avatar=None, + name=room_name, + avatar=room_avatar, # TODO: Dummy value heroes=None, # TODO: Dummy value is_dm=False, initial=initial, - required_state=list(room_state.values()) if room_state else None, + required_state=( + list(required_room_state.values()) if required_room_state else None + ), timeline_events=timeline_events, bundled_aggregations=bundled_aggregations, stripped_state=stripped_state, diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 6ff1f03c9a..6ba17eea0b 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -2973,6 +2973,7 @@ def test_rooms_required_state_initial_sync(self) -> None: }, exact=True, ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) def test_rooms_required_state_incremental_sync(self) -> None: """ @@ -3027,6 +3028,7 @@ def test_rooms_required_state_incremental_sync(self) -> None: }, exact=True, ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) def test_rooms_required_state_wildcard(self) -> None: """ @@ -3084,6 +3086,7 @@ def test_rooms_required_state_wildcard(self) -> None: state_map.values(), exact=True, ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) def test_rooms_required_state_wildcard_event_type(self) -> None: """ @@ -3147,6 +3150,7 @@ def test_rooms_required_state_wildcard_event_type(self) -> None: # events when the `event_type` is a wildcard. exact=False, ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) def test_rooms_required_state_wildcard_state_key(self) -> None: """ @@ -3192,6 +3196,7 @@ def test_rooms_required_state_wildcard_state_key(self) -> None: }, exact=True, ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) def test_rooms_required_state_lazy_loading_room_members(self) -> None: """ @@ -3247,6 +3252,7 @@ def test_rooms_required_state_lazy_loading_room_members(self) -> None: }, exact=True, ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) @parameterized.expand([(Membership.LEAVE,), (Membership.BAN,)]) def test_rooms_required_state_leave_ban(self, stop_membership: str) -> None: @@ -3329,6 +3335,7 @@ def test_rooms_required_state_leave_ban(self, stop_membership: str) -> None: }, exact=True, ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) def test_rooms_required_state_combine_superset(self) -> None: """ @@ -3401,6 +3408,7 @@ def test_rooms_required_state_combine_superset(self) -> None: }, exact=True, ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) def test_rooms_required_state_partial_state(self) -> None: """ From b0bb37fcf02c9d0dea4466ae6d90f56583405934 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 8 Jul 2024 16:23:29 -0500 Subject: [PATCH 02/39] Add tests --- tests/rest/client/test_sync.py | 182 +++++++++++++++++++++++++++++++++ 1 file changed, 182 insertions(+) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 6ba17eea0b..02d27c3a64 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -1802,6 +1802,188 @@ def test_sliced_windows(self) -> None: channel.json_body["lists"]["foo-list"], ) + def test_rooms_meta_when_joined(self) -> None: + """ + Test that the `rooms` `name` and `avatar` (soon to test `heroes`) are included + in the response when the user is joined to 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") + + room_id1 = self.helper.create_room_as( + user2_id, + tok=user2_tok, + extra_content={ + "name": "my super room", + }, + ) + # Set the room avatar URL + self.helper.send_state( + room_id1, + EventTypes.RoomAvatar, + {"url": "mxc://DUMMY_MEDIA_ID"}, + tok=user2_tok, + ) + + self.helper.join(room_id1, user1_id, tok=user1_tok) + + # Make the Sliding Sync request + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + self.assertEqual( + channel.json_body["rooms"][room_id1]["name"], + "my super room", + channel.json_body["rooms"][room_id1], + ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["avatar"], + "mxc://DUMMY_MEDIA_ID", + channel.json_body["rooms"][room_id1], + ) + + def test_rooms_meta_when_invited(self) -> None: + """ + Test that the `rooms` `name` and `avatar` (soon to test `heroes`) are included + in the response when the user is invited to 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") + + room_id1 = self.helper.create_room_as( + user2_id, + tok=user2_tok, + extra_content={ + "name": "my super room", + }, + ) + # Set the room avatar URL + self.helper.send_state( + room_id1, + EventTypes.RoomAvatar, + {"url": "mxc://DUMMY_MEDIA_ID"}, + tok=user2_tok, + ) + + self.helper.join(room_id1, user1_id, tok=user1_tok) + + # Make the Sliding Sync request + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + self.assertEqual( + channel.json_body["rooms"][room_id1]["name"], + "my super room", + channel.json_body["rooms"][room_id1], + ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["avatar"], + "mxc://DUMMY_MEDIA_ID", + channel.json_body["rooms"][room_id1], + ) + + def test_rooms_meta_when_banned(self) -> None: + """ + Test that the `rooms` `name` and `avatar` (soon to test `heroes`) reflect the + state of the room when the user was banned (do not leak current state). + """ + 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") + + room_id1 = self.helper.create_room_as( + user2_id, + tok=user2_tok, + extra_content={ + "name": "my super room", + }, + ) + # Set the room avatar URL + self.helper.send_state( + room_id1, + EventTypes.RoomAvatar, + {"url": "mxc://DUMMY_MEDIA_ID"}, + tok=user2_tok, + ) + + self.helper.join(room_id1, user1_id, tok=user1_tok) + self.helper.ban(room_id1, src=user2_id, targ=user1_id, tok=user2_tok) + + # Update the room name after user1 has left + self.helper.send_state( + room_id1, + EventTypes.Name, + {"name": "my super duper room"}, + tok=user2_tok, + ) + # Update the room avatar URL after user1 has left + self.helper.send_state( + room_id1, + EventTypes.RoomAvatar, + {"url": "mxc://UPDATED_DUMMY_MEDIA_ID"}, + tok=user2_tok, + ) + + # Make the Sliding Sync request + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Reflect the state of the room at the time of leaving + self.assertEqual( + channel.json_body["rooms"][room_id1]["name"], + "my super room", + channel.json_body["rooms"][room_id1], + ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["avatar"], + "mxc://DUMMY_MEDIA_ID", + channel.json_body["rooms"][room_id1], + ) + def test_rooms_limited_initial_sync(self) -> None: """ Test that we mark `rooms` as `limited=True` when we saturate the `timeline_limit` From b6e36ef65880370e2981e02199f7630827183f99 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 8 Jul 2024 16:26:05 -0500 Subject: [PATCH 03/39] Add changelog --- changelog.d/17418.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17418.feature diff --git a/changelog.d/17418.feature b/changelog.d/17418.feature new file mode 100644 index 0000000000..c5e56bc500 --- /dev/null +++ b/changelog.d/17418.feature @@ -0,0 +1 @@ +Populate `name`/`avatar` fields in experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint. From 6ef39dd233701e401f3f7cf30920d7ee076853e2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 8 Jul 2024 16:28:10 -0500 Subject: [PATCH 04/39] Actually test that invited should see current state --- tests/rest/client/test_sync.py | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 02d27c3a64..f7852562b1 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -1846,6 +1846,7 @@ def test_rooms_meta_when_joined(self) -> None: ) self.assertEqual(channel.code, 200, channel.json_body) + # Reflect the current state of the room self.assertEqual( channel.json_body["rooms"][room_id1]["name"], "my super room", @@ -1884,6 +1885,21 @@ def test_rooms_meta_when_invited(self) -> None: self.helper.join(room_id1, user1_id, tok=user1_tok) + # Update the room name after user1 has left + self.helper.send_state( + room_id1, + EventTypes.Name, + {"name": "my super duper room"}, + tok=user2_tok, + ) + # Update the room avatar URL after user1 has left + self.helper.send_state( + room_id1, + EventTypes.RoomAvatar, + {"url": "mxc://UPDATED_DUMMY_MEDIA_ID"}, + tok=user2_tok, + ) + # Make the Sliding Sync request channel = self.make_request( "POST", @@ -1901,14 +1917,16 @@ def test_rooms_meta_when_invited(self) -> None: ) self.assertEqual(channel.code, 200, channel.json_body) + # This should still reflect the current state of the room even when the user is + # invited. self.assertEqual( channel.json_body["rooms"][room_id1]["name"], - "my super room", + "my super duper room", channel.json_body["rooms"][room_id1], ) self.assertEqual( channel.json_body["rooms"][room_id1]["avatar"], - "mxc://DUMMY_MEDIA_ID", + "mxc://UPDATED_DUMMY_MEDIA_ID", channel.json_body["rooms"][room_id1], ) From 32c5409ded2638ec1c83290d8f2a78556bcc1837 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 8 Jul 2024 18:40:25 -0500 Subject: [PATCH 05/39] Start of heroes in Sliding Sync --- synapse/handlers/sliding_sync.py | 246 +++++++++++++------ synapse/storage/databases/main/roommember.py | 31 ++- synapse/types/handlers/__init__.py | 7 +- synapse/types/rest/client/__init__.py | 4 - 4 files changed, 198 insertions(+), 90 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index bb81ca9d97..b854d41f68 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -19,7 +19,7 @@ # import logging from itertools import chain -from typing import TYPE_CHECKING, Any, Dict, Final, List, Optional, Set, Tuple +from typing import TYPE_CHECKING, Any, Dict, Final, List, Mapping, Optional, Set, Tuple import attr from immutabledict import immutabledict @@ -34,7 +34,9 @@ from synapse.events import EventBase from synapse.events.utils import strip_event from synapse.handlers.relations import BundledAggregations +from synapse.storage.databases.main.roommember import extract_heroes_from_room_summary from synapse.storage.databases.main.stream import CurrentStateDeltaMembership +from synapse.storage.roommember import MemberSummary from synapse.types import ( JsonDict, PersistedEventPosition, @@ -1041,6 +1043,103 @@ async def sort_rooms( reverse=True, ) + async def get_current_state_ids_at( + self, + room_id: str, + room_membership_for_user_at_to_token: _RoomMembershipForUser, + state_filter: StateFilter, + to_token: StreamToken, + ) -> StateMap[str]: + """ + Get current state IDs for the user in the room according to their membership. This + will be the current state at the time of their LEAVE/BAN, otherwise will be the + current state <= to_token. + + Args: + room_id: The room ID to fetch data for + room_membership_for_user_at_token: Membership information for the user + in the room at the time of `to_token`. + to_token: The point in the stream to sync up to. + """ + + room_state_ids: StateMap[str] + # People shouldn't see past their leave/ban event + if room_membership_for_user_at_to_token.membership in ( + Membership.LEAVE, + Membership.BAN, + ): + # TODO: `get_state_ids_at(...)` doesn't take into account the "current state" + room_state_ids = await self.storage_controllers.state.get_state_ids_at( + room_id, + stream_position=to_token.copy_and_replace( + StreamKeyType.ROOM, + room_membership_for_user_at_to_token.event_pos.to_room_stream_token(), + ), + state_filter=state_filter, + # Partially-stated rooms should have all state events except for + # remote membership events. Since we've already excluded + # partially-stated rooms unless `required_state` only has + # `["m.room.member", "$LAZY"]` for membership, we should be able to + # retrieve everything requested. When we're lazy-loading, if there + # are some remote senders in the timeline, we should also have their + # membership event because we had to auth that timeline event. Plus + # we don't want to block the whole sync waiting for this one room. + await_full_state=False, + ) + # Otherwise, we can get the latest current state in the room + else: + room_state_ids = await self.storage_controllers.state.get_current_state_ids( + room_id, + state_filter, + # Partially-stated rooms should have all state events except for + # remote membership events. Since we've already excluded + # partially-stated rooms unless `required_state` only has + # `["m.room.member", "$LAZY"]` for membership, we should be able to + # retrieve everything requested. When we're lazy-loading, if there + # are some remote senders in the timeline, we should also have their + # membership event because we had to auth that timeline event. Plus + # we don't want to block the whole sync waiting for this one room. + await_full_state=False, + ) + # TODO: Query `current_state_delta_stream` and reverse/rewind back to the `to_token` + + return room_state_ids + + async def get_current_state_at( + self, + room_id: str, + room_membership_for_user_at_to_token: _RoomMembershipForUser, + state_filter: StateFilter, + to_token: StreamToken, + ) -> StateMap[EventBase]: + """ + Get current state for the user in the room according to their membership. This + will be the current state at the time of their LEAVE/BAN, otherwise will be the + current state <= to_token. + + Args: + room_id: The room ID to fetch data for + room_membership_for_user_at_token: Membership information for the user + in the room at the time of `to_token`. + to_token: The point in the stream to sync up to. + """ + room_state_ids = await self.get_current_state_ids_at( + room_id=room_id, + room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, + state_filter=state_filter, + to_token=to_token, + ) + + event_map = await self.store.get_events(list(room_state_ids.values())) + + state_map = {} + for key, event_id in room_state_ids.items(): + event = event_map.get(event_id) + if event: + state_map[key] = event + + return state_map + async def get_room_sync_data( self, user: UserID, @@ -1241,6 +1340,34 @@ async def get_room_sync_data( # updates. initial = True + # Check whether the room has a name set + name_state_ids = await self.get_current_state_ids_at( + room_id=room_id, + room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, + state_filter=StateFilter.from_types([(EventTypes.Name, "")]), + to_token=to_token, + ) + name_event_id = name_state_ids.get((EventTypes.Name, "")) + + room_membership_summary: Mapping[str, MemberSummary] + empty_membership_summary = MemberSummary([], 0) + if room_membership_for_user_at_to_token.membership in ( + Membership.LEAVE, + Membership.BAN, + ): + # TODO: Figure out how to get the membership summary for left/banned rooms + room_membership_summary = {} + else: + room_membership_summary = await self.store.get_room_summary(room_id) + # TODO: Reverse/rewind back to the `to_token` + + # `heroes` are required if the room name is not set + hero_user_ids: Optional[List[str]] = None + if name_event_id is None: + hero_user_ids = extract_heroes_from_room_summary( + room_membership_summary, me=user.to_string() + ) + # Fetch the `required_state` for the room # # No `required_state` for invite/knock rooms (just `stripped_state`) @@ -1252,12 +1379,18 @@ async def get_room_sync_data( # # Calculate the `StateFilter` based on the `required_state` for the room room_state: Optional[StateMap[EventBase]] = None - required_room_state: Optional[StateMap[EventBase]] = None + # Start off with the heroes of the room + required_state_filter = ( + StateFilter.from_types( + [(EventTypes.Member, hero_user_id) for hero_user_id in hero_user_ids] + ) + if hero_user_ids + else StateFilter.none() + ) if room_membership_for_user_at_to_token.membership not in ( Membership.INVITE, Membership.KNOCK, ): - required_state_filter = StateFilter.none() # If we have a double wildcard ("*", "*") in the `required_state`, we need # to fetch all state for the room # @@ -1321,66 +1454,37 @@ async def get_room_sync_data( else: required_state_types.append((state_type, state_key)) - required_state_filter = StateFilter.from_types(required_state_types) + required_state_filter = StateFilter.from_types( + chain(required_state_types, required_state_filter.to_types()) + ) + + # We need this base set of info for the response so let's just fetch it along + # with the `required_state` for the room + meta_room_state = [(EventTypes.Name, ""), (EventTypes.RoomAvatar, "")] + state_filter = StateFilter( + types=StateFilter.from_types( + chain(meta_room_state, required_state_filter.to_types()) + ).types, + include_others=required_state_filter.include_others, + ) - # We need this base set of info for the response so let's just fetch it along - # with the `required_state` for the room - META_ROOM_STATE = [(EventTypes.Name, ""), (EventTypes.RoomAvatar, "")] - state_filter = StateFilter( - types=StateFilter.from_types( - chain(META_ROOM_STATE, required_state_filter.to_types()) - ).types, - include_others=required_state_filter.include_others, + # We can return all of the state that was requested if this was the first + # time we've sent the room down this connection. + if initial: + room_state = await self.get_current_state_at( + room_id=room_id, + room_membership_for_user_at_to_token=room_membership_for_user_at_to_token, + state_filter=state_filter, + to_token=to_token, ) + else: + # TODO: Once we can figure out if we've sent a room down this connection before, + # we can return updates instead of the full required state. + raise NotImplementedError() - # We can return all of the state that was requested if this was the first - # time we've sent the room down this connection. - if initial: - # People shouldn't see past their leave/ban event - if room_membership_for_user_at_to_token.membership in ( - Membership.LEAVE, - Membership.BAN, - ): - room_state = await self.storage_controllers.state.get_state_at( - room_id, - stream_position=to_token.copy_and_replace( - StreamKeyType.ROOM, - room_membership_for_user_at_to_token.event_pos.to_room_stream_token(), - ), - state_filter=state_filter, - # Partially-stated rooms should have all state events except for - # remote membership events. Since we've already excluded - # partially-stated rooms unless `required_state` only has - # `["m.room.member", "$LAZY"]` for membership, we should be able to - # retrieve everything requested. When we're lazy-loading, if there - # are some remote senders in the timeline, we should also have their - # membership event because we had to auth that timeline event. Plus - # we don't want to block the whole sync waiting for this one room. - await_full_state=False, - ) - # Otherwise, we can get the latest current state in the room - else: - room_state = await self.storage_controllers.state.get_current_state( - room_id, - state_filter, - # Partially-stated rooms should have all state events except for - # remote membership events. Since we've already excluded - # partially-stated rooms unless `required_state` only has - # `["m.room.member", "$LAZY"]` for membership, we should be able to - # retrieve everything requested. When we're lazy-loading, if there - # are some remote senders in the timeline, we should also have their - # membership event because we had to auth that timeline event. Plus - # we don't want to block the whole sync waiting for this one room. - await_full_state=False, - ) - # TODO: Query `current_state_delta_stream` and reverse/rewind back to the `to_token` - else: - # TODO: Once we can figure out if we've sent a room down this connection before, - # we can return updates instead of the full required state. - raise NotImplementedError() - - if required_state_filter != StateFilter.none(): - required_room_state = required_state_filter.filter_state(room_state) + required_room_state: Optional[StateMap[EventBase]] = None + if required_state_filter != StateFilter.none(): + required_room_state = required_state_filter.filter_state(room_state) # Find the room name and avatar from the state room_name: Optional[str] = None @@ -1393,16 +1497,6 @@ async def get_room_sync_data( avatar_event = room_state.get((EventTypes.RoomAvatar, "")) if avatar_event is not None: room_avatar = avatar_event.content.get("url") - elif stripped_state is not None: - for event in stripped_state: - if event["type"] == EventTypes.Name: - room_name = event.get("content", {}).get("name") - elif event["type"] == EventTypes.RoomAvatar: - room_avatar = event.get("content", {}).get("url") - - # Found everything so we can stop looking - if room_name is not None and room_avatar is not None: - break # Figure out the last bump event in the room last_bump_event_result = ( @@ -1421,8 +1515,7 @@ async def get_room_sync_data( return SlidingSyncResult.RoomResult( name=room_name, avatar=room_avatar, - # TODO: Dummy value - heroes=None, + heroes=hero_user_ids, # TODO: Dummy value is_dm=False, initial=initial, @@ -1436,9 +1529,12 @@ async def get_room_sync_data( limited=limited, num_live=num_live, bump_stamp=bump_stamp, - # TODO: Dummy values - joined_count=0, - invited_count=0, + joined_count=room_membership_summary.get( + Membership.JOIN, empty_membership_summary + ).count, + invited_count=room_membership_summary.get( + Membership.INVITE, empty_membership_summary + ).count, # TODO: These are just dummy values. We could potentially just remove these # since notifications can only really be done correctly on the client anyway # (encrypted rooms). diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index d8b54dc4e3..4e161847ab 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -284,8 +284,19 @@ def _get_users_in_room_with_profiles( @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: @@ -313,23 +324,27 @@ def _get_room_summary_txn( 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/knocks -> everything else), 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 2 ELSE 3 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, Membership.JOIN, Membership.INVITE, Membership.KNOCK, 6) + ) for user_id, membership, event_id in txn: summary = res[membership] # we will always have a summary for this membership type at this diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index 43dcdf20dd..9da3b88cc2 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -154,8 +154,9 @@ class RoomResult: Attributes: name: Room name or calculated room name. avatar: Room avatar - heroes: List of stripped membership events (containing `user_id` and optionally - `avatar_url` and `displayname`) for the users used to calculate the room name. + heroes: List of user_ids which can be used to generate a room name if the + room does not have one. The `required_state` will include the membership + events for these users. is_dm: Flag to specify whether the room is a direct-message room (most likely between two people). initial: Flag which is set when this is the first time the server is sending this @@ -202,7 +203,7 @@ class RoomResult: name: Optional[str] avatar: Optional[str] - heroes: Optional[List[EventBase]] + heroes: Optional[List[str]] is_dm: bool initial: bool # Only optional because it won't be included for invite/knock rooms with `stripped_state` diff --git a/synapse/types/rest/client/__init__.py b/synapse/types/rest/client/__init__.py index 55f6b44053..bef5cb9cb9 100644 --- a/synapse/types/rest/client/__init__.py +++ b/synapse/types/rest/client/__init__.py @@ -200,9 +200,6 @@ class SlidingSyncList(CommonRoomParameters): } timeline_limit: The maximum number of timeline events to return per response. - include_heroes: Return a stripped variant of membership events (containing - `user_id` and optionally `avatar_url` and `displayname`) for the users used - to calculate the room name. filters: Filters to apply to the list before sorting. """ @@ -270,7 +267,6 @@ class Filters(RequestBodyModel): else: ranges: Optional[List[Tuple[conint(ge=0, strict=True), conint(ge=0, strict=True)]]] = None # type: ignore[valid-type] slow_get_all_rooms: Optional[StrictBool] = False - include_heroes: Optional[StrictBool] = False filters: Optional[Filters] = None class RoomSubscription(CommonRoomParameters): From f58d6fcd9bb6b394b33efa47a5cdffca49309baf Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 17:01:48 -0500 Subject: [PATCH 06/39] `heroes` is not `required_state` for now Discussion tracking this: https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1669441698 It's also hard because we may not have any full membership events when you're the first one on your server to be invited to a new room over federation. --- synapse/handlers/sliding_sync.py | 75 +++++++++++++++++++----------- synapse/rest/client/sync.py | 28 +++++++++-- synapse/types/handlers/__init__.py | 23 +++++---- 3 files changed, 84 insertions(+), 42 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index b854d41f68..2429264b9b 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -1171,7 +1171,7 @@ async def get_room_sync_data( # membership. Currently, we have to make all of these optional because # `invite`/`knock` rooms only have `stripped_state`. See # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1653045932 - timeline_events: Optional[List[EventBase]] = None + timeline_events: List[EventBase] = [] bundled_aggregations: Optional[Dict[str, BundledAggregations]] = None limited: Optional[bool] = None prev_batch_token: Optional[StreamToken] = None @@ -1303,7 +1303,7 @@ async def get_room_sync_data( # Figure out any stripped state events for invite/knocks. This allows the # potential joiner to identify the room. - stripped_state: Optional[List[JsonDict]] = None + stripped_state: List[JsonDict] = [] if room_membership_for_user_at_to_token.membership in ( Membership.INVITE, Membership.KNOCK, @@ -1361,8 +1361,18 @@ async def get_room_sync_data( room_membership_summary = await self.store.get_room_summary(room_id) # TODO: Reverse/rewind back to the `to_token` - # `heroes` are required if the room name is not set - hero_user_ids: Optional[List[str]] = None + # `heroes` are required if the room name is not set. + # + # Note: When you're the first one on your server to be invited to a new room + # over federation, we only have access to some stripped state in + # `event.unsigned.invite_room_state` which currently doesn't include `heroes`, + # see https://github.com/matrix-org/matrix-spec/issues/380. This means that + # clients won't be able to calculate the room name when necessary and just a + # pitfall we have to deal with until that spec issue is resolved. + hero_user_ids: List[str] = [] + # TODO: Should we also check for `EventTypes.CanonicalAlias` + # (`m.room.canonical_alias`) as a fallback for the room name? see + # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153 if name_event_id is None: hero_user_ids = extract_heroes_from_room_summary( room_membership_summary, me=user.to_string() @@ -1378,15 +1388,7 @@ async def get_room_sync_data( # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1653045932 # # Calculate the `StateFilter` based on the `required_state` for the room - room_state: Optional[StateMap[EventBase]] = None - # Start off with the heroes of the room - required_state_filter = ( - StateFilter.from_types( - [(EventTypes.Member, hero_user_id) for hero_user_id in hero_user_ids] - ) - if hero_user_ids - else StateFilter.none() - ) + required_state_filter = StateFilter.none() if room_membership_for_user_at_to_token.membership not in ( Membership.INVITE, Membership.KNOCK, @@ -1454,22 +1456,25 @@ async def get_room_sync_data( else: required_state_types.append((state_type, state_key)) - required_state_filter = StateFilter.from_types( - chain(required_state_types, required_state_filter.to_types()) - ) + required_state_filter = StateFilter.from_types(required_state_types) # We need this base set of info for the response so let's just fetch it along # with the `required_state` for the room - meta_room_state = [(EventTypes.Name, ""), (EventTypes.RoomAvatar, "")] - state_filter = StateFilter( - types=StateFilter.from_types( - chain(meta_room_state, required_state_filter.to_types()) - ).types, - include_others=required_state_filter.include_others, - ) + meta_room_state = [(EventTypes.Name, ""), (EventTypes.RoomAvatar, "")] + [ + (EventTypes.Member, hero_user_id) for hero_user_id in hero_user_ids + ] + state_filter = StateFilter.all() + if required_state_filter != StateFilter.all(): + state_filter = StateFilter( + types=StateFilter.from_types( + chain(meta_room_state, required_state_filter.to_types()) + ).types, + include_others=required_state_filter.include_others, + ) # We can return all of the state that was requested if this was the first # time we've sent the room down this connection. + room_state: StateMap[EventBase] = {} if initial: room_state = await self.get_current_state_at( room_id=room_id, @@ -1482,7 +1487,7 @@ async def get_room_sync_data( # we can return updates instead of the full required state. raise NotImplementedError() - required_room_state: Optional[StateMap[EventBase]] = None + required_room_state: StateMap[EventBase] = {} if required_state_filter != StateFilter.none(): required_room_state = required_state_filter.filter_state(room_state) @@ -1490,6 +1495,9 @@ async def get_room_sync_data( room_name: Optional[str] = None room_avatar: Optional[str] = None if room_state is not None: + # TODO: Should we also check for `EventTypes.CanonicalAlias` + # (`m.room.canonical_alias`) as a fallback for the room name? see + # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153 name_event = room_state.get((EventTypes.Name, "")) if name_event is not None: room_name = name_event.content.get("name") @@ -1498,6 +1506,19 @@ async def get_room_sync_data( if avatar_event is not None: room_avatar = avatar_event.content.get("url") + # Assemble heroes: extract the info from the state we just fetched + heroes: List[SlidingSyncResult.RoomResult.StrippedHero] = [] + for hero_user_id in hero_user_ids: + member_event = room_state.get((EventTypes.Member, hero_user_id)) + if member_event is not None: + heroes.append( + SlidingSyncResult.RoomResult.StrippedHero( + user_id=hero_user_id, + display_name=member_event.content.get("displayname"), + avatar_url=member_event.content.get("avatar_url"), + ) + ) + # Figure out the last bump event in the room last_bump_event_result = ( await self.store.get_last_event_pos_in_room_before_stream_ordering( @@ -1515,13 +1536,11 @@ async def get_room_sync_data( return SlidingSyncResult.RoomResult( name=room_name, avatar=room_avatar, - heroes=hero_user_ids, + heroes=heroes, # TODO: Dummy value is_dm=False, initial=initial, - required_state=( - list(required_room_state.values()) if required_room_state else None - ), + required_state=list(required_room_state.values()), timeline_events=timeline_events, bundled_aggregations=bundled_aggregations, stripped_state=stripped_state, diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 13aed1dc85..966b42ca1f 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -995,8 +995,17 @@ async def encode_rooms( if room_result.avatar: serialized_rooms[room_id]["avatar"] = room_result.avatar - if room_result.heroes: - serialized_rooms[room_id]["heroes"] = room_result.heroes + if room_result.heroes is not None and len(room_result.heroes) > 0: + serialized_heroes = [] + for hero in room_result.heroes: + serialized_heroes.append( + { + "user_id": hero.user_id, + "displayname": hero.display_name, + "avatar_url": hero.avatar_url, + } + ) + serialized_rooms[room_id]["heroes"] = serialized_heroes # We should only include the `initial` key if it's `True` to save bandwidth. # The absense of this flag means `False`. @@ -1004,7 +1013,10 @@ async def encode_rooms( serialized_rooms[room_id]["initial"] = room_result.initial # This will be omitted for invite/knock rooms with `stripped_state` - if room_result.required_state is not None: + if ( + room_result.required_state is not None + and len(room_result.required_state) > 0 + ): serialized_required_state = ( await self.event_serializer.serialize_events( room_result.required_state, @@ -1015,7 +1027,10 @@ async def encode_rooms( serialized_rooms[room_id]["required_state"] = serialized_required_state # This will be omitted for invite/knock rooms with `stripped_state` - if room_result.timeline_events is not None: + if ( + room_result.timeline_events is not None + and len(room_result.timeline_events) > 0 + ): serialized_timeline = await self.event_serializer.serialize_events( room_result.timeline_events, time_now, @@ -1043,7 +1058,10 @@ async def encode_rooms( serialized_rooms[room_id]["is_dm"] = room_result.is_dm # Stripped state only applies to invite/knock rooms - if room_result.stripped_state is not None: + if ( + room_result.stripped_state is not None + and len(room_result.stripped_state) > 0 + ): # TODO: `knocked_state` but that isn't specced yet. # # TODO: Instead of adding `knocked_state`, it would be good to rename diff --git a/synapse/types/handlers/__init__.py b/synapse/types/handlers/__init__.py index 9da3b88cc2..bee992fd2f 100644 --- a/synapse/types/handlers/__init__.py +++ b/synapse/types/handlers/__init__.py @@ -154,9 +154,8 @@ class RoomResult: Attributes: name: Room name or calculated room name. avatar: Room avatar - heroes: List of user_ids which can be used to generate a room name if the - room does not have one. The `required_state` will include the membership - events for these users. + heroes: List of stripped membership events (containing `user_id` and optionally + `avatar_url` and `displayname`) for the users used to calculate the room name. is_dm: Flag to specify whether the room is a direct-message room (most likely between two people). initial: Flag which is set when this is the first time the server is sending this @@ -201,18 +200,24 @@ class RoomResult: flag set. (same as sync v2) """ + @attr.s(slots=True, frozen=True, auto_attribs=True) + class StrippedHero: + user_id: str + display_name: Optional[str] + avatar_url: Optional[str] + name: Optional[str] avatar: Optional[str] - heroes: Optional[List[str]] + heroes: Optional[List[StrippedHero]] is_dm: bool initial: bool - # Only optional because it won't be included for invite/knock rooms with `stripped_state` - required_state: Optional[List[EventBase]] - # Only optional because it won't be included for invite/knock rooms with `stripped_state` - timeline_events: Optional[List[EventBase]] + # Should be empty for invite/knock rooms with `stripped_state` + required_state: List[EventBase] + # Should be empty for invite/knock rooms with `stripped_state` + timeline_events: List[EventBase] bundled_aggregations: Optional[Dict[str, "BundledAggregations"]] # Optional because it's only relevant to invite/knock rooms - stripped_state: Optional[List[JsonDict]] + stripped_state: List[JsonDict] # Only optional because it won't be included for invite/knock rooms with `stripped_state` prev_batch: Optional[StreamToken] # Only optional because it won't be included for invite/knock rooms with `stripped_state` From 82bf80c9393f7c7b8dcdd279f83de5220905bd26 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 17:10:35 -0500 Subject: [PATCH 07/39] Add changelog --- changelog.d/17419.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17419.feature diff --git a/changelog.d/17419.feature b/changelog.d/17419.feature new file mode 100644 index 0000000000..94716a6e92 --- /dev/null +++ b/changelog.d/17419.feature @@ -0,0 +1 @@ +Populate `heroes` field in experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint. From 10f8540bfad666860b46da1fe09d33bb4d7c9f0f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 17:31:50 -0500 Subject: [PATCH 08/39] Add tests --- synapse/rest/client/sync.py | 18 ++++--- tests/rest/client/test_sync.py | 88 +++++++++++++++++++++++++++++----- 2 files changed, 88 insertions(+), 18 deletions(-) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index 966b42ca1f..94fc16b139 100644 --- a/synapse/rest/client/sync.py +++ b/synapse/rest/client/sync.py @@ -998,13 +998,17 @@ async def encode_rooms( if room_result.heroes is not None and len(room_result.heroes) > 0: serialized_heroes = [] for hero in room_result.heroes: - serialized_heroes.append( - { - "user_id": hero.user_id, - "displayname": hero.display_name, - "avatar_url": hero.avatar_url, - } - ) + serialized_hero = { + "user_id": hero.user_id, + } + if hero.display_name is not None: + # Not a typo, just how "displayname" is spelled in the spec + serialized_hero["displayname"] = hero.display_name + + if hero.avatar_url is not None: + serialized_hero["avatar_url"] = hero.avatar_url + + serialized_heroes.append(serialized_hero) serialized_rooms[room_id]["heroes"] = serialized_heroes # We should only include the `initial` key if it's `True` to save bandwidth. diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index f7852562b1..209fe637db 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -1804,8 +1804,8 @@ def test_sliced_windows(self) -> None: def test_rooms_meta_when_joined(self) -> None: """ - Test that the `rooms` `name` and `avatar` (soon to test `heroes`) are included - in the response when the user is joined to the room. + Test that the `rooms` `name` and `avatar` are included in the response and + reflect the current state of the room when the user is joined to the room. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1860,8 +1860,8 @@ def test_rooms_meta_when_joined(self) -> None: def test_rooms_meta_when_invited(self) -> None: """ - Test that the `rooms` `name` and `avatar` (soon to test `heroes`) are included - in the response when the user is invited to the room. + Test that the `rooms` `name` and `avatar` are included in the response and + reflect the current state of the room when the user is invited to the room. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1932,8 +1932,8 @@ def test_rooms_meta_when_invited(self) -> None: def test_rooms_meta_when_banned(self) -> None: """ - Test that the `rooms` `name` and `avatar` (soon to test `heroes`) reflect the - state of the room when the user was banned (do not leak current state). + Test that the `rooms` `name` and `avatar` reflect the state of the room when the + user was banned (do not leak current state). """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -2002,6 +2002,76 @@ def test_rooms_meta_when_banned(self) -> None: channel.json_body["rooms"][room_id1], ) + def test_rooms_meta_heroes(self) -> None: + """ + Test that the `rooms` `heroes` are included in the response when the room + doesn't have a room name set. + """ + 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") + + room_id1 = self.helper.create_room_as( + user2_id, + tok=user2_tok, + extra_content={ + "name": "my super room", + }, + ) + self.helper.join(room_id1, user1_id, tok=user1_tok) + room_id2 = self.helper.create_room_as( + user2_id, + tok=user2_tok, + extra_content={ + # We don't want a room name set so that `heroes` is populated + # + # "name": "my super room2", + }, + ) + self.helper.join(room_id2, user1_id, tok=user1_tok) + + # Make the Sliding Sync request + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Room1 has a name so we shouldn't see any `heroes` which the client would use + # the calculate the room name themselves. + self.assertEqual( + channel.json_body["rooms"][room_id1]["name"], + "my super room", + channel.json_body["rooms"][room_id1], + ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("heroes")) + + # Room2 doesn't have a name so we should see `heroes` populated + self.assertIsNone(channel.json_body["rooms"][room_id2].get("name")) + self.assertCountEqual( + [ + hero["user_id"] + for hero in channel.json_body["rooms"][room_id2].get("heroes", []) + ], + # Heroes shouldn't include the user themselves (we shouldn't see user1) + [user2_id], + ) + + # We didn't request any state so we shouldn't see any `required_state` + self.assertIsNone(channel.json_body["rooms"][room_id1].get("required_state")) + self.assertIsNone(channel.json_body["rooms"][room_id2].get("required_state")) + def test_rooms_limited_initial_sync(self) -> None: """ Test that we mark `rooms` as `limited=True` when we saturate the `timeline_limit` @@ -3072,11 +3142,7 @@ def test_rooms_ban_incremental_sync2(self) -> None: self.assertEqual(channel.code, 200, channel.json_body) # Nothing to see for this banned user in the room in the token range - self.assertEqual( - channel.json_body["rooms"][room_id1]["timeline"], - [], - channel.json_body["rooms"][room_id1]["timeline"], - ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("timeline")) # No events returned in the timeline so nothing is "live" self.assertEqual( channel.json_body["rooms"][room_id1]["num_live"], From 62925b6ca5bc3aaf778fcef01fcb80a7edf451fd Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 17:41:49 -0500 Subject: [PATCH 09/39] Remove unncessary check --- synapse/handlers/sliding_sync.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 2429264b9b..08a5dcd971 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -1493,18 +1493,17 @@ async def get_room_sync_data( # Find the room name and avatar from the state room_name: Optional[str] = None + # TODO: Should we also check for `EventTypes.CanonicalAlias` + # (`m.room.canonical_alias`) as a fallback for the room name? see + # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153 + name_event = room_state.get((EventTypes.Name, "")) + if name_event is not None: + room_name = name_event.content.get("name") + room_avatar: Optional[str] = None - if room_state is not None: - # TODO: Should we also check for `EventTypes.CanonicalAlias` - # (`m.room.canonical_alias`) as a fallback for the room name? see - # https://github.com/matrix-org/matrix-spec-proposals/pull/3575#discussion_r1671260153 - name_event = room_state.get((EventTypes.Name, "")) - if name_event is not None: - room_name = name_event.content.get("name") - - avatar_event = room_state.get((EventTypes.RoomAvatar, "")) - if avatar_event is not None: - room_avatar = avatar_event.content.get("url") + avatar_event = room_state.get((EventTypes.RoomAvatar, "")) + if avatar_event is not None: + room_avatar = avatar_event.content.get("url") # Assemble heroes: extract the info from the state we just fetched heroes: List[SlidingSyncResult.RoomResult.StrippedHero] = [] From b9f1eb1d6189a9eab446f89a22f9591945bae3aa Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 17:57:19 -0500 Subject: [PATCH 10/39] Test `joined_count`/`invited_count` --- changelog.d/17419.feature | 2 +- tests/rest/client/test_sync.py | 126 ++++++++++++++++++++++++++++++++- 2 files changed, 125 insertions(+), 3 deletions(-) diff --git a/changelog.d/17419.feature b/changelog.d/17419.feature index 94716a6e92..186a27c470 100644 --- a/changelog.d/17419.feature +++ b/changelog.d/17419.feature @@ -1 +1 @@ -Populate `heroes` field in experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint. +Populate `heroes` and room summary fields (`joined_count`, `invited_count`) in experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint. diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 209fe637db..25c78c25bb 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -1857,6 +1857,14 @@ def test_rooms_meta_when_joined(self) -> None: "mxc://DUMMY_MEDIA_ID", channel.json_body["rooms"][room_id1], ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["joined_count"], + 2, + ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["invited_count"], + 0, + ) def test_rooms_meta_when_invited(self) -> None: """ @@ -1883,7 +1891,8 @@ def test_rooms_meta_when_invited(self) -> None: tok=user2_tok, ) - self.helper.join(room_id1, user1_id, tok=user1_tok) + # User1 is invited to the room + self.helper.invite(room_id1, src=user2_id, targ=user1_id, tok=user2_tok) # Update the room name after user1 has left self.helper.send_state( @@ -1929,6 +1938,14 @@ def test_rooms_meta_when_invited(self) -> None: "mxc://UPDATED_DUMMY_MEDIA_ID", channel.json_body["rooms"][room_id1], ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["joined_count"], + 1, + ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["invited_count"], + 1, + ) def test_rooms_meta_when_banned(self) -> None: """ @@ -2001,6 +2018,16 @@ def test_rooms_meta_when_banned(self) -> None: "mxc://DUMMY_MEDIA_ID", channel.json_body["rooms"][room_id1], ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["joined_count"], + # FIXME: The actual number should be "1" (user2) but we currently don't + # support this for rooms where the user has left/been banned. + 0, + ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["invited_count"], + 0, + ) def test_rooms_meta_heroes(self) -> None: """ @@ -2024,7 +2051,7 @@ def test_rooms_meta_heroes(self) -> None: user2_id, tok=user2_tok, extra_content={ - # We don't want a room name set so that `heroes` is populated + # No room name set so that `heroes` is populated # # "name": "my super room2", }, @@ -2056,6 +2083,14 @@ def test_rooms_meta_heroes(self) -> None: channel.json_body["rooms"][room_id1], ) self.assertIsNone(channel.json_body["rooms"][room_id1].get("heroes")) + self.assertEqual( + channel.json_body["rooms"][room_id1]["joined_count"], + 2, + ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["invited_count"], + 0, + ) # Room2 doesn't have a name so we should see `heroes` populated self.assertIsNone(channel.json_body["rooms"][room_id2].get("name")) @@ -2067,11 +2102,98 @@ def test_rooms_meta_heroes(self) -> None: # Heroes shouldn't include the user themselves (we shouldn't see user1) [user2_id], ) + self.assertEqual( + channel.json_body["rooms"][room_id2]["joined_count"], + 2, + ) + self.assertEqual( + channel.json_body["rooms"][room_id2]["invited_count"], + 0, + ) # We didn't request any state so we shouldn't see any `required_state` self.assertIsNone(channel.json_body["rooms"][room_id1].get("required_state")) self.assertIsNone(channel.json_body["rooms"][room_id2].get("required_state")) + def test_rooms_meta_heroes_when_banned(self) -> None: + """ + Test that the `rooms` `heroes` are included in the response when the room + doesn't have a room name set but doesn't leak information past their ban. + """ + 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") + + room_id1 = self.helper.create_room_as( + user2_id, + tok=user2_tok, + extra_content={ + # No room name set so that `heroes` is populated + # + # "name": "my super room", + }, + ) + # User1 joins the room + self.helper.join(room_id1, user1_id, tok=user1_tok) + # User3 is invited + self.helper.invite(room_id1, src=user2_id, targ=user3_id, tok=user2_tok) + + # User1 is banned from the room + self.helper.ban(room_id1, src=user2_id, targ=user1_id, tok=user2_tok) + + # User4 joins the room after user1 is banned + self.helper.join(room_id1, user4_id, tok=user4_tok) + + # Make the Sliding Sync request + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Room2 doesn't have a name so we should see `heroes` populated + self.assertIsNone(channel.json_body["rooms"][room_id1].get("name")) + self.assertCountEqual( + [ + hero["user_id"] + for hero in channel.json_body["rooms"][room_id1].get("heroes", []) + ], + # Heroes shouldn't include the user themselves (we shouldn't see user1). We + # also shouldn't see user4 since they joined after user1 was banned. + # + # FIXME: The actual result should be `[user2_id, user3_id]` but we currently + # don't support this for rooms where the user has left/been banned. + [], + ) + + self.assertEqual( + channel.json_body["rooms"][room_id1]["joined_count"], + # FIXME: The actual number should be "1" (user2) but we currently don't + # support this for rooms where the user has left/been banned. + 0, + ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["invited_count"], + # FIXME: The actual number should be "1" (user3) but we currently don't + # support this for rooms where the user has left/been banned. + 0, + ) + def test_rooms_limited_initial_sync(self) -> None: """ Test that we mark `rooms` as `limited=True` when we saturate the `timeline_limit` From e50bf86ba2b083f43b8ae19da01e964876367e5e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 17:59:24 -0500 Subject: [PATCH 11/39] Test invite before/after ban --- tests/rest/client/test_sync.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 25c78c25bb..4e5b438884 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -2125,9 +2125,11 @@ def test_rooms_meta_heroes_when_banned(self) -> None: 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") + _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") room_id1 = self.helper.create_room_as( user2_id, @@ -2148,6 +2150,8 @@ def test_rooms_meta_heroes_when_banned(self) -> None: # User4 joins the room after user1 is banned self.helper.join(room_id1, user4_id, tok=user4_tok) + # User5 is invited after user1 is banned + self.helper.invite(room_id1, src=user2_id, targ=user5_id, tok=user2_tok) # Make the Sliding Sync request channel = self.make_request( @@ -2189,6 +2193,8 @@ def test_rooms_meta_heroes_when_banned(self) -> None: ) self.assertEqual( channel.json_body["rooms"][room_id1]["invited_count"], + # We shouldn't see user5 since they were invited after user1 was banned. + # # FIXME: The actual number should be "1" (user3) but we currently don't # support this for rooms where the user has left/been banned. 0, From 2fb77b36f59fd24e4df1aaced15177220f3a6a42 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 18:08:19 -0500 Subject: [PATCH 12/39] Sort `leave` before `knock` See https://github.com/element-hq/synapse/pull/17419#discussion_r1671337103 --- synapse/storage/databases/main/roommember.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 2bf0d13be8..62bd2a1d38 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -319,7 +319,7 @@ def _get_room_summary_txn( for count, membership in txn: res.setdefault(membership, MemberSummary([], count)) - # Order by membership (joins -> invites/knocks -> everything else), then by + # Order by membership (joins -> invites -> leave -> everything else), then by # `stream_ordering` so the first members in the room show up first and to # make the sort stable (consistent heroes). # @@ -331,14 +331,14 @@ def _get_room_summary_txn( WHERE type = 'm.room.member' AND room_id = ? AND membership IS NOT NULL ORDER BY - CASE membership WHEN ? THEN 1 WHEN ? THEN 2 WHEN ? THEN 2 ELSE 3 END 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, Membership.KNOCK, 6) + sql, (room_id, Membership.JOIN, Membership.INVITE, Membership.LEAVE, 6) ) for user_id, membership, event_id in txn: summary = res[membership] From 275da50286c0f6565a62195b4bfa858d50c9a680 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 18:10:05 -0500 Subject: [PATCH 13/39] Explain the order --- synapse/storage/databases/main/roommember.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 62bd2a1d38..1d13de62f0 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -319,9 +319,10 @@ def _get_room_summary_txn( for count, membership in txn: res.setdefault(membership, MemberSummary([], count)) - # Order by membership (joins -> invites -> leave -> everything else), then by - # `stream_ordering` so the first members in the room show up first and to - # make the sort stable (consistent heroes). + # Order by membership (joins -> invites -> leave (former insiders) -> everything else + # including knocks since they are outsiders), 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. From 6060408fa345139173124b17b41754758b98f96e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 18:23:05 -0500 Subject: [PATCH 14/39] Add test to make sure we only return 5 heroes --- tests/rest/client/test_sync.py | 88 ++++++++++++++++++++++++++++++++-- 1 file changed, 85 insertions(+), 3 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 4e5b438884..5b85a14ea8 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -2038,6 +2038,8 @@ def test_rooms_meta_heroes(self) -> None: 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") room_id1 = self.helper.create_room_as( user2_id, @@ -2047,6 +2049,9 @@ def test_rooms_meta_heroes(self) -> None: }, ) self.helper.join(room_id1, user1_id, tok=user1_tok) + # User3 is invited + self.helper.invite(room_id1, src=user2_id, targ=user3_id, tok=user2_tok) + room_id2 = self.helper.create_room_as( user2_id, tok=user2_tok, @@ -2057,6 +2062,8 @@ def test_rooms_meta_heroes(self) -> None: }, ) self.helper.join(room_id2, user1_id, tok=user1_tok) + # User3 is invited + self.helper.invite(room_id2, src=user2_id, targ=user3_id, tok=user2_tok) # Make the Sliding Sync request channel = self.make_request( @@ -2089,7 +2096,7 @@ def test_rooms_meta_heroes(self) -> None: ) self.assertEqual( channel.json_body["rooms"][room_id1]["invited_count"], - 0, + 1, ) # Room2 doesn't have a name so we should see `heroes` populated @@ -2100,7 +2107,7 @@ def test_rooms_meta_heroes(self) -> None: for hero in channel.json_body["rooms"][room_id2].get("heroes", []) ], # Heroes shouldn't include the user themselves (we shouldn't see user1) - [user2_id], + [user2_id, user3_id], ) self.assertEqual( channel.json_body["rooms"][room_id2]["joined_count"], @@ -2108,13 +2115,88 @@ def test_rooms_meta_heroes(self) -> None: ) self.assertEqual( channel.json_body["rooms"][room_id2]["invited_count"], - 0, + 1, ) # We didn't request any state so we shouldn't see any `required_state` self.assertIsNone(channel.json_body["rooms"][room_id1].get("required_state")) self.assertIsNone(channel.json_body["rooms"][room_id2].get("required_state")) + def test_rooms_meta_heroes_max(self) -> None: + """ + Test that the `rooms` `heroes` only includes the first 5 users (not including + yourself). + """ + 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") + + room_id1 = self.helper.create_room_as( + user2_id, + tok=user2_tok, + extra_content={ + # No room name set so that `heroes` is populated + # + # "name": "my super room", + }, + ) + self.helper.join(room_id1, user1_id, tok=user1_tok) + self.helper.join(room_id1, user3_id, tok=user3_tok) + self.helper.join(room_id1, user4_id, tok=user4_tok) + self.helper.join(room_id1, user5_id, tok=user5_tok) + self.helper.join(room_id1, user6_id, tok=user6_tok) + self.helper.join(room_id1, user7_id, tok=user7_tok) + + # Make the Sliding Sync request + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [], + "timeline_limit": 0, + } + } + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # Room2 doesn't have a name so we should see `heroes` populated + self.assertIsNone(channel.json_body["rooms"][room_id1].get("name")) + self.assertCountEqual( + [ + hero["user_id"] + for hero in channel.json_body["rooms"][room_id1].get("heroes", []) + ], + # Heroes shouldn't include 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, + ) + self.assertEqual( + channel.json_body["rooms"][room_id1]["invited_count"], + 0, + ) + + # We didn't request any state so we shouldn't see any `required_state` + self.assertIsNone(channel.json_body["rooms"][room_id1].get("required_state")) + def test_rooms_meta_heroes_when_banned(self) -> None: """ Test that the `rooms` `heroes` are included in the response when the room From 91cefaac458be4c35abd232b8eeb3a25af955fef Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 18:27:15 -0500 Subject: [PATCH 15/39] Fix lint --- tests/rest/client/test_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 5b85a14ea8..674d966903 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -2039,7 +2039,7 @@ def test_rooms_meta_heroes(self) -> None: 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") + _user3_tok = self.login(user3_id, "pass") room_id1 = self.helper.create_room_as( user2_id, From 4205159b4396708ea4f2bf14385789cb7d48afb1 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 21:16:55 -0500 Subject: [PATCH 16/39] Add `is_dm` fields to Sliding Sync --- synapse/handlers/sliding_sync.py | 75 ++++++++++++++++++++------------ 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 08a5dcd971..51e6a2bd1c 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -297,6 +297,7 @@ class _RoomMembershipForUser: sender: The person who sent the membership event newly_joined: Whether the user newly joined the room during the given token range + is_dm: Whether this user considers this room as a direct-message (DM) room """ room_id: str @@ -305,6 +306,7 @@ class _RoomMembershipForUser: membership: str sender: Optional[str] newly_joined: bool + is_dm: bool def copy_and_replace(self, **kwds: Any) -> "_RoomMembershipForUser": return attr.evolve(self, **kwds) @@ -615,6 +617,7 @@ async def get_sync_room_ids_for_user( membership=room_for_user.membership, sender=room_for_user.sender, newly_joined=False, + is_dm=False, ) for room_for_user in room_for_user_list } @@ -654,6 +657,7 @@ async def get_sync_room_ids_for_user( # - 1c) Update room membership events to the point in time of the `to_token` # - 2) Add back newly_left rooms (> `from_token` and <= `to_token`) # - 3) Figure out which rooms are `newly_joined` + # - 4) Figure out which rooms are DM's # 1) ----------------------------------------------------- @@ -716,6 +720,7 @@ async def get_sync_room_ids_for_user( membership=first_membership_change_after_to_token.prev_membership, sender=first_membership_change_after_to_token.prev_sender, newly_joined=False, + is_dm=False, ) else: # If we can't find the previous membership event, we shouldn't @@ -811,6 +816,7 @@ async def get_sync_room_ids_for_user( membership=last_membership_change_in_from_to_range.membership, sender=last_membership_change_in_from_to_range.sender, newly_joined=False, + is_dm=False, ) # 3) Figure out `newly_joined` @@ -848,6 +854,35 @@ async def get_sync_room_ids_for_user( room_id ].copy_and_replace(newly_joined=True) + # 4) Figure out which rooms the user considers to be direct-message (DM) rooms + # + # We're using global account data (`m.direct`) instead of checking for + # `is_direct` on membership events because that property only appears for + # the invitee membership event (doesn't show up for the inviter). + # + # We're unable to take `to_token` into account for global account data since + # we only keep track of the latest account data for the user. + dm_map = await self.store.get_global_account_data_by_type_for_user( + user_id, AccountDataTypes.DIRECT + ) + + # Flatten out the map. Account data is set by the client so it needs to be + # scrutinized. + dm_room_id_set = set() + if isinstance(dm_map, dict): + for room_ids in dm_map.values(): + # Account data should be a list of room IDs. Ignore anything else + if isinstance(room_ids, list): + for room_id in room_ids: + if isinstance(room_id, str): + dm_room_id_set.add(room_id) + + # 4) Fixup + for room_id in filtered_sync_room_id_set: + filtered_sync_room_id_set[room_id] = filtered_sync_room_id_set[ + room_id + ].copy_and_replace(is_dm=room_id in dm_room_id_set) + return filtered_sync_room_id_set async def filter_rooms( @@ -871,41 +906,24 @@ async def filter_rooms( A filtered dictionary of room IDs along with membership information in the room at the time of `to_token`. """ - user_id = user.to_string() - - # TODO: Apply filters - filtered_room_id_set = set(sync_room_map.keys()) # Filter for Direct-Message (DM) rooms if filters.is_dm is not None: - # We're using global account data (`m.direct`) instead of checking for - # `is_direct` on membership events because that property only appears for - # the invitee membership event (doesn't show up for the inviter). Account - # data is set by the client so it needs to be scrutinized. - # - # We're unable to take `to_token` into account for global account data since - # we only keep track of the latest account data for the user. - dm_map = await self.store.get_global_account_data_by_type_for_user( - user_id, AccountDataTypes.DIRECT - ) - - # Flatten out the map - dm_room_id_set = set() - if isinstance(dm_map, dict): - for room_ids in dm_map.values(): - # Account data should be a list of room IDs. Ignore anything else - if isinstance(room_ids, list): - for room_id in room_ids: - if isinstance(room_id, str): - dm_room_id_set.add(room_id) - if filters.is_dm: # Only DM rooms please - filtered_room_id_set = filtered_room_id_set.intersection(dm_room_id_set) + filtered_room_id_set = { + room_id + for room_id in filtered_room_id_set + if sync_room_map[room_id].is_dm + } else: # Only non-DM rooms please - filtered_room_id_set = filtered_room_id_set.difference(dm_room_id_set) + filtered_room_id_set = { + room_id + for room_id in filtered_room_id_set + if not sync_room_map[room_id].is_dm + } if filters.spaces: raise NotImplementedError() @@ -1536,8 +1554,7 @@ async def get_room_sync_data( name=room_name, avatar=room_avatar, heroes=heroes, - # TODO: Dummy value - is_dm=False, + is_dm=room_membership_for_user_at_to_token.is_dm, initial=initial, required_state=list(required_room_state.values()), timeline_events=timeline_events, From 1bd953ba3da3ed2f401289ccac20f7bb75d0a975 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 21:24:28 -0500 Subject: [PATCH 17/39] Add tests for `is_dm` --- tests/rest/client/test_sync.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 674d966903..bec4858acd 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -1653,6 +1653,20 @@ def test_filter_list(self) -> None: list(channel.json_body["lists"]["room-invites"]), ) + # Ensure DM's are correctly marked + self.assertDictEqual( + { + room_id: room.get("is_dm") + for room_id, room in channel.json_body["rooms"].items() + }, + { + invite_room_id: None, + room_id: None, + invited_dm_room_id: True, + joined_dm_room_id: True, + }, + ) + def test_sort_list(self) -> None: """ Test that the `lists` are sorted by `stream_ordering` @@ -1865,6 +1879,9 @@ def test_rooms_meta_when_joined(self) -> None: channel.json_body["rooms"][room_id1]["invited_count"], 0, ) + self.assertIsNone( + channel.json_body["rooms"][room_id1].get("is_dm"), + ) def test_rooms_meta_when_invited(self) -> None: """ @@ -1946,6 +1963,9 @@ def test_rooms_meta_when_invited(self) -> None: channel.json_body["rooms"][room_id1]["invited_count"], 1, ) + self.assertIsNone( + channel.json_body["rooms"][room_id1].get("is_dm"), + ) def test_rooms_meta_when_banned(self) -> None: """ @@ -2028,6 +2048,9 @@ def test_rooms_meta_when_banned(self) -> None: channel.json_body["rooms"][room_id1]["invited_count"], 0, ) + self.assertIsNone( + channel.json_body["rooms"][room_id1].get("is_dm"), + ) def test_rooms_meta_heroes(self) -> None: """ From 8681b7dfbcc7a6de51c4b6b4c9828a3f33f28c65 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 9 Jul 2024 21:28:09 -0500 Subject: [PATCH 18/39] Add changelog --- changelog.d/17429.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17429.feature diff --git a/changelog.d/17429.feature b/changelog.d/17429.feature new file mode 100644 index 0000000000..608b75d632 --- /dev/null +++ b/changelog.d/17429.feature @@ -0,0 +1 @@ +Populate `is_dm` room field in experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint. From 479dc072f4ea0a9fa38d644e16cf36eabc8dc9ae Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 10 Jul 2024 16:00:10 -0500 Subject: [PATCH 19/39] Start of room subscriptions --- synapse/handlers/sliding_sync.py | 296 +++++++++++++++++++++---------- 1 file changed, 198 insertions(+), 98 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 51e6a2bd1c..38f7d2e27f 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -30,6 +30,7 @@ EventContentFields, EventTypes, Membership, + HistoryVisibility, ) from synapse.events import EventBase from synapse.events.utils import strip_event @@ -68,19 +69,48 @@ } +@attr.s(slots=True, frozen=True, auto_attribs=True) +class _RoomMembershipForUser: + """ + Attributes: + event_id: The event ID of the membership event + event_pos: The stream position of the membership event + membership: The membership state of the user in the room + sender: The person who sent the membership event + newly_joined: Whether the user newly joined the room during the given token + range + is_dm: Whether this user considers this room as a direct-message (DM) room + """ + + room_id: str + event_id: Optional[str] + event_pos: PersistedEventPosition + membership: str + sender: Optional[str] + newly_joined: bool + is_dm: bool + + def copy_and_replace(self, **kwds: Any) -> "_RoomMembershipForUser": + return attr.evolve(self, **kwds) + + def filter_membership_for_sync( - *, membership: str, user_id: str, sender: Optional[str] + *, user_id: str, room_membership_for_user: _RoomMembershipForUser ) -> bool: """ Returns True if the membership event should be included in the sync response, otherwise False. Attributes: - membership: The membership state of the user in the room. user_id: The user ID that the membership applies to - sender: The person who sent the membership event + room_membership_for_user: Membership information for the user in the room """ + membership = room_membership_for_user.membership + sender = room_membership_for_user.sender + # TODO + newly_left = room_membership_for_user.newly_left + # Everything except `Membership.LEAVE` because we want everything that's *still* # relevant to the user. There are few more things to include in the sync response # (newly_left) but those are handled separately. @@ -287,31 +317,6 @@ class StateValues: LAZY: Final = "$LAZY" -@attr.s(slots=True, frozen=True, auto_attribs=True) -class _RoomMembershipForUser: - """ - Attributes: - event_id: The event ID of the membership event - event_pos: The stream position of the membership event - membership: The membership state of the user in the room - sender: The person who sent the membership event - newly_joined: Whether the user newly joined the room during the given token - range - is_dm: Whether this user considers this room as a direct-message (DM) room - """ - - room_id: str - event_id: Optional[str] - event_pos: PersistedEventPosition - membership: str - sender: Optional[str] - newly_joined: bool - is_dm: bool - - def copy_and_replace(self, **kwds: Any) -> "_RoomMembershipForUser": - return attr.evolve(self, **kwds) - - class SlidingSyncHandler: def __init__(self, hs: "HomeServer"): self.clock = hs.get_clock() @@ -530,7 +535,34 @@ async def current_sync_for_user( ops=ops, ) - # TODO: if (sync_config.room_subscriptions): + # Handle room subscriptions + if sync_config.room_subscriptions is not None: + for room_id, room_subscription in sync_config.room_subscriptions.items(): + # We can first check if they are already allowed to see the room based + # on our previous work to assemble the `sync_room_map`. + if sync_room_map.get(room_id) is None: + # If not, we have to do some work to figure out if they should be + # allowed to see the room. + room_membership_for_user_at_to_token = ( + await self.check_room_subscription_allowed_for_user( + user=sync_config.user, room_id=room_id + ) + ) + + # Skip this room if the user isn't allowed to see it + if not room_membership_for_user_at_to_token: + continue + + # Take the superset of the `RoomSyncConfig` for each room. + # + # Update our `relevant_room_map` with the room we're going to display + # and need to fetch more info about. + room_sync_config = RoomSyncConfig.from_room_config(room_subscription) + existing_room_sync_config = relevant_room_map.get(room_id) + if existing_room_sync_config is not None: + existing_room_sync_config.combine_room_sync_config(room_sync_config) + else: + relevant_room_map[room_id] = room_sync_config # Fetch room data rooms: Dict[str, SlidingSyncResult.RoomResult] = {} @@ -553,7 +585,7 @@ async def current_sync_for_user( extensions={}, ) - async def get_sync_room_ids_for_user( + async def get_membership_for_user_at_to_token( self, user: UserID, to_token: StreamToken, @@ -563,18 +595,13 @@ async def get_sync_room_ids_for_user( Fetch room IDs that should be listed for this user in the sync response (the full room list that will be filtered, sorted, and sliced). - We're looking for rooms where the user has the following state in the token - range (> `from_token` and <= `to_token`): + We're looking for rooms where the user has had any sort of membership in the + token range (> `from_token` and <= `to_token`): - - `invite`, `join`, `knock`, `ban` membership events - - Kicks (`leave` membership events where `sender` is different from the - `user_id`/`state_key`) - - `newly_left` (rooms that were left during the given token range) - - In order for bans/kicks to not show up in sync, you need to `/forget` those - rooms. This doesn't modify the event itself though and only adds the - `forgotten` flag to the `room_memberships` table in Synapse. There isn't a way - to tell when a room was forgotten at the moment so we can't factor it into the - from/to range. + - In order for bans/kicks to not show up, you need to `/forget` those rooms. + This doesn't modify the event itself though and only adds the `forgotten` flag + to the `room_memberships` table in Synapse. There isn't a way to tell when a + room was forgotten at the moment so we can't factor it into the token range. Args: user: User to fetch rooms for @@ -582,8 +609,8 @@ async def get_sync_room_ids_for_user( from_token: The point in the stream to sync from. Returns: - A dictionary of room IDs that should be listed in the sync response along - with membership information in that room at the time of `to_token`. + A dictionary of room IDs that the user has had membership in along with + membership information in that room at the time of `to_token`. """ user_id = user.to_string() @@ -594,9 +621,6 @@ async def get_sync_room_ids_for_user( # We want to fetch any kind of membership (joined and left rooms) in order # to get the `event_pos` of the latest room membership event for the # user. - # - # We will filter out the rooms that don't belong below (see - # `filter_membership_for_sync`) membership_list=Membership.LIST, excluded_rooms=self.rooms_to_exclude_globally, ) @@ -616,6 +640,7 @@ async def get_sync_room_ids_for_user( event_pos=room_for_user.event_pos, membership=room_for_user.membership, sender=room_for_user.sender, + # We will update these fields below to be accurate newly_joined=False, is_dm=False, ) @@ -648,16 +673,15 @@ async def get_sync_room_ids_for_user( instance_map=immutabledict(instance_to_max_stream_ordering_map), ) - # Since we fetched the users room list at some point in time after the from/to - # tokens, we need to revert/rewind some membership changes to match the point in - # time of the `to_token`. In particular, we need to make these fixups: + # Since we fetched the users room list at some point in time after `to_token`, + # we need to revert/rewind some membership changes to match the point in time of + # the `to_token`. In particular, we need to make these fixups: # # - 1a) Remove rooms that the user joined after the `to_token` # - 1b) Add back rooms that the user left after the `to_token` # - 1c) Update room membership events to the point in time of the `to_token` - # - 2) Add back newly_left rooms (> `from_token` and <= `to_token`) - # - 3) Figure out which rooms are `newly_joined` - # - 4) Figure out which rooms are DM's + # - 2) Figure out which rooms are `newly_joined` + # - 3) Figure out which rooms are DM's # 1) ----------------------------------------------------- @@ -728,22 +752,6 @@ async def get_sync_room_ids_for_user( # exact membership state and shouldn't rely on the current snapshot. sync_room_id_set.pop(room_id, None) - # Filter the rooms that that we have updated room membership events to the point - # in time of the `to_token` (from the "1)" fixups) - filtered_sync_room_id_set = { - room_id: room_membership_for_user - for room_id, room_membership_for_user in sync_room_id_set.items() - if filter_membership_for_sync( - membership=room_membership_for_user.membership, - user_id=user_id, - sender=room_membership_for_user.sender, - ) - } - - # 2) ----------------------------------------------------- - # We fix-up newly_left rooms after the first fixup because it may have removed - # some left rooms that we can figure out are newly_left in the following code - # 2) Fetch membership changes that fall in the range from `from_token` up to `to_token` current_state_delta_membership_changes_in_from_to_range = [] if from_token: @@ -789,9 +797,7 @@ async def get_sync_room_ids_for_user( if membership_change.membership != Membership.JOIN: has_non_join_event_by_room_id_in_from_to_range[room_id] = True - # 2) Fixup - # - # 3) We also want to assemble a list of possibly newly joined rooms. Someone + # 2) We also want to assemble a list of possibly newly joined rooms. Someone # could have left and joined multiple times during the given range but we only # care about whether they are joined at the end of the token range so we are # working with the last membership even in the token range. @@ -801,25 +807,11 @@ async def get_sync_room_ids_for_user( ) in last_membership_change_by_room_id_in_from_to_range.values(): room_id = last_membership_change_in_from_to_range.room_id - # 3) + # 2) if last_membership_change_in_from_to_range.membership == Membership.JOIN: possibly_newly_joined_room_ids.add(room_id) - # 2) Add back newly_left rooms (> `from_token` and <= `to_token`). We - # include newly_left rooms because the last event that the user should see - # is their own leave event - if last_membership_change_in_from_to_range.membership == Membership.LEAVE: - filtered_sync_room_id_set[room_id] = _RoomMembershipForUser( - room_id=room_id, - event_id=last_membership_change_in_from_to_range.event_id, - event_pos=last_membership_change_in_from_to_range.event_pos, - membership=last_membership_change_in_from_to_range.membership, - sender=last_membership_change_in_from_to_range.sender, - newly_joined=False, - is_dm=False, - ) - - # 3) Figure out `newly_joined` + # 2) Figure out `newly_joined` for room_id in possibly_newly_joined_room_ids: has_non_join_in_from_to_range = ( has_non_join_event_by_room_id_in_from_to_range.get(room_id, False) @@ -828,9 +820,9 @@ async def get_sync_room_ids_for_user( # also some non-join in the range, we know they `newly_joined`. if has_non_join_in_from_to_range: # We found a `newly_joined` room (we left and joined within the token range) - filtered_sync_room_id_set[room_id] = filtered_sync_room_id_set[ - room_id - ].copy_and_replace(newly_joined=True) + sync_room_id_set[room_id] = sync_room_id_set[room_id].copy_and_replace( + newly_joined=True + ) else: prev_event_id = first_membership_change_by_room_id_in_from_to_range[ room_id @@ -842,7 +834,7 @@ async def get_sync_room_ids_for_user( if prev_event_id is None: # We found a `newly_joined` room (we are joining the room for the # first time within the token range) - filtered_sync_room_id_set[room_id] = filtered_sync_room_id_set[ + sync_room_id_set[room_id] = sync_room_id_set[ room_id ].copy_and_replace(newly_joined=True) # Last resort, we need to step back to the previous membership event @@ -850,11 +842,11 @@ async def get_sync_room_ids_for_user( elif prev_membership != Membership.JOIN: # We found a `newly_joined` room (we left before the token range # and joined within the token range) - filtered_sync_room_id_set[room_id] = filtered_sync_room_id_set[ + sync_room_id_set[room_id] = sync_room_id_set[ room_id ].copy_and_replace(newly_joined=True) - # 4) Figure out which rooms the user considers to be direct-message (DM) rooms + # 3) Figure out which rooms the user considers to be direct-message (DM) rooms # # We're using global account data (`m.direct`) instead of checking for # `is_direct` on membership events because that property only appears for @@ -877,14 +869,122 @@ async def get_sync_room_ids_for_user( if isinstance(room_id, str): dm_room_id_set.add(room_id) - # 4) Fixup - for room_id in filtered_sync_room_id_set: - filtered_sync_room_id_set[room_id] = filtered_sync_room_id_set[ - room_id - ].copy_and_replace(is_dm=room_id in dm_room_id_set) + # 3) Fixup + for room_id in sync_room_id_set: + sync_room_id_set[room_id] = sync_room_id_set[room_id].copy_and_replace( + is_dm=room_id in dm_room_id_set + ) + + return sync_room_id_set + + async def get_sync_room_ids_for_user( + self, + user: UserID, + to_token: StreamToken, + from_token: Optional[StreamToken] = None, + ) -> Dict[str, _RoomMembershipForUser]: + """ + Fetch room IDs that should be listed for this user in the sync response (the + full room list that will be filtered, sorted, and sliced). + + We're looking for rooms where the user has the following state in the token + range (> `from_token` and <= `to_token`): + + - `invite`, `join`, `knock`, `ban` membership events + - Kicks (`leave` membership events where `sender` is different from the + `user_id`/`state_key`) + - `newly_left` (rooms that were left during the given token range) + - In order for bans/kicks to not show up in sync, you need to `/forget` those + rooms. This doesn't modify the event itself though and only adds the + `forgotten` flag to the `room_memberships` table in Synapse. There isn't a way + to tell when a room was forgotten at the moment so we can't factor it into the + from/to range. + + Args: + user: User to fetch rooms for + to_token: The token to fetch rooms up to. + from_token: The point in the stream to sync from. + + Returns: + A dictionary of room IDs that should be listed in the sync response along + with membership information in that room at the time of `to_token`. + """ + user_id = user.to_string() + + sync_room_id_set = await self.get_membership_for_user_at_to_token( + user=user, + to_token=to_token, + ) + + # Filter the rooms + filtered_sync_room_id_set = { + room_id: room_membership_for_user + for room_id, room_membership_for_user in sync_room_id_set.items() + if filter_membership_for_sync( + user_id=user_id, + room_membership_for_user=room_membership_for_user, + ) + } return filtered_sync_room_id_set + async def check_room_subscription_allowed_for_user( + self, user: UserID, room_id: str + ) -> Optional[_RoomMembershipForUser]: + """ + Check whether the user is allowed to see the room based on whether they have + ever had membership in the room or if the room is `world_readable`. + + Similar to `check_user_in_room_or_world_readable(...)` + + Args: + TODO + + Returns: + TODO + """ + user_id = user.to_string() + + # If they have had any membership in the room over time, let them + # subscribe and see what they can. + ( + membership, + member_event_id, + ) = await self.store.get_local_current_membership_for_user_in_room( + user_id=user_id, + room_id=room_id, + ) + if membership is not None and member_event_id is not None: + return _RoomMembershipForUser( + room_id=room_id, + event_id=member_event_id, + event_pos=room_for_user.event_pos, + membership=membership, + sender=room_for_user.sender, + newly_joined=False, + is_dm=False, + ) + + # If the room is `world_readable`, it doesn't matter whether they can join, + # everyone can see the room. + visibility = await self.storage_controllers.state.get_current_state_event( + room_id, EventTypes.RoomHistoryVisibility, "" + ) + if ( + visibility + and visibility.content.get("history_visibility") + == HistoryVisibility.WORLD_READABLE + ): + return _RoomMembershipForUser( + room_id=room_id, + event_id=None, + event_pos=None, + membership=None, + sender=None, + newly_joined=False, + is_dm=False, + ) + async def filter_rooms( self, user: UserID, From 6f9ff152b23e2169ea3fa2cb8a011c0ab718f6f2 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 10 Jul 2024 16:27:05 -0500 Subject: [PATCH 20/39] Split fetching membership from filtering for sync --- synapse/handlers/sliding_sync.py | 168 ++++++++++++++++------------ tests/handlers/test_sliding_sync.py | 10 +- 2 files changed, 98 insertions(+), 80 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 38f7d2e27f..95371facc1 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -29,8 +29,8 @@ Direction, EventContentFields, EventTypes, - Membership, HistoryVisibility, + Membership, ) from synapse.events import EventBase from synapse.events.utils import strip_event @@ -79,6 +79,7 @@ class _RoomMembershipForUser: sender: The person who sent the membership event newly_joined: Whether the user newly joined the room during the given token range + newly_left: Whether the user newly left the room during the given token range is_dm: Whether this user considers this room as a direct-message (DM) room """ @@ -88,6 +89,7 @@ class _RoomMembershipForUser: membership: str sender: Optional[str] newly_joined: bool + newly_left: bool is_dm: bool def copy_and_replace(self, **kwds: Any) -> "_RoomMembershipForUser": @@ -108,22 +110,21 @@ def filter_membership_for_sync( membership = room_membership_for_user.membership sender = room_membership_for_user.sender - # TODO newly_left = room_membership_for_user.newly_left - # Everything except `Membership.LEAVE` because we want everything that's *still* - # relevant to the user. There are few more things to include in the sync response - # (newly_left) but those are handled separately. + # We want to allow everything except rooms the user has left unless `newly_left` + # because we want everything that's *still* relevant to the user. # - # This logic includes kicks (leave events where the sender is not the same user) and - # can be read as "anything that isn't a leave or a leave with a different sender". + # A leave != kick. This logic includes kicks (leave events where the sender is not + # the same user) and can be read as "anything that isn't a leave or newly_left or a + # leave with a different sender". # # When `sender=None` and `membership=Membership.LEAVE`, it means that a state reset # happened that removed the user from the room, or the user was the last person # locally to leave the room which caused the server to leave the room. In both # cases, we can just remove the rooms since they are no longer relevant to the user. # They could still be added back later if they are `newly_left`. - return membership != Membership.LEAVE or sender not in (user_id, None) + return membership != Membership.LEAVE or newly_left or sender not in (user_id, None) # We can't freeze this class because we want to update it in place with the @@ -536,33 +537,33 @@ async def current_sync_for_user( ) # Handle room subscriptions - if sync_config.room_subscriptions is not None: - for room_id, room_subscription in sync_config.room_subscriptions.items(): - # We can first check if they are already allowed to see the room based - # on our previous work to assemble the `sync_room_map`. - if sync_room_map.get(room_id) is None: - # If not, we have to do some work to figure out if they should be - # allowed to see the room. - room_membership_for_user_at_to_token = ( - await self.check_room_subscription_allowed_for_user( - user=sync_config.user, room_id=room_id - ) - ) - - # Skip this room if the user isn't allowed to see it - if not room_membership_for_user_at_to_token: - continue - - # Take the superset of the `RoomSyncConfig` for each room. - # - # Update our `relevant_room_map` with the room we're going to display - # and need to fetch more info about. - room_sync_config = RoomSyncConfig.from_room_config(room_subscription) - existing_room_sync_config = relevant_room_map.get(room_id) - if existing_room_sync_config is not None: - existing_room_sync_config.combine_room_sync_config(room_sync_config) - else: - relevant_room_map[room_id] = room_sync_config + # if sync_config.room_subscriptions is not None: + # for room_id, room_subscription in sync_config.room_subscriptions.items(): + # # We can first check if they are already allowed to see the room based + # # on our previous work to assemble the `sync_room_map`. + # if sync_room_map.get(room_id) is None: + # # If not, we have to do some work to figure out if they should be + # # allowed to see the room. + # room_membership_for_user_at_to_token = ( + # await self.check_room_subscription_allowed_for_user( + # user=sync_config.user, room_id=room_id + # ) + # ) + + # # Skip this room if the user isn't allowed to see it + # if not room_membership_for_user_at_to_token: + # continue + + # # Take the superset of the `RoomSyncConfig` for each room. + # # + # # Update our `relevant_room_map` with the room we're going to display + # # and need to fetch more info about. + # room_sync_config = RoomSyncConfig.from_room_config(room_subscription) + # existing_room_sync_config = relevant_room_map.get(room_id) + # if existing_room_sync_config is not None: + # existing_room_sync_config.combine_room_sync_config(room_sync_config) + # else: + # relevant_room_map[room_id] = room_sync_config # Fetch room data rooms: Dict[str, SlidingSyncResult.RoomResult] = {} @@ -589,19 +590,19 @@ async def get_membership_for_user_at_to_token( self, user: UserID, to_token: StreamToken, - from_token: Optional[StreamToken] = None, + from_token: Optional[StreamToken], ) -> Dict[str, _RoomMembershipForUser]: """ - Fetch room IDs that should be listed for this user in the sync response (the - full room list that will be filtered, sorted, and sliced). + Fetch room IDs that the user has had membership in (the full room list that will + be filtered, sorted, and sliced). We're looking for rooms where the user has had any sort of membership in the - token range (> `from_token` and <= `to_token`): + token range (> `from_token` and <= `to_token`) - - In order for bans/kicks to not show up, you need to `/forget` those rooms. - This doesn't modify the event itself though and only adds the `forgotten` flag - to the `room_memberships` table in Synapse. There isn't a way to tell when a - room was forgotten at the moment so we can't factor it into the token range. + In order for bans/kicks to not show up, you need to `/forget` those rooms. This + doesn't modify the event itself though and only adds the `forgotten` flag to the + `room_memberships` table in Synapse. There isn't a way to tell when a room was + forgotten at the moment so we can't factor it into the token range. Args: user: User to fetch rooms for @@ -642,6 +643,7 @@ async def get_membership_for_user_at_to_token( sender=room_for_user.sender, # We will update these fields below to be accurate newly_joined=False, + newly_left=False, is_dm=False, ) for room_for_user in room_for_user_list @@ -673,17 +675,16 @@ async def get_membership_for_user_at_to_token( instance_map=immutabledict(instance_to_max_stream_ordering_map), ) - # Since we fetched the users room list at some point in time after `to_token`, - # we need to revert/rewind some membership changes to match the point in time of - # the `to_token`. In particular, we need to make these fixups: + # Since we fetched the users room list at some point in time after the from/to + # tokens, we need to revert/rewind some membership changes to match the point in + # time of the `to_token`. In particular, we need to make these fixups: # # - 1a) Remove rooms that the user joined after the `to_token` # - 1b) Add back rooms that the user left after the `to_token` # - 1c) Update room membership events to the point in time of the `to_token` - # - 2) Figure out which rooms are `newly_joined` - # - 3) Figure out which rooms are DM's - - # 1) ----------------------------------------------------- + # - 2) Figure out which rooms are `newly_left` rooms (> `from_token` and <= `to_token`) + # - 3) Figure out which rooms are `newly_joined` + # - 4) Figure out which rooms are DM's # 1) Fetch membership changes that fall in the range from `to_token` up to # `membership_snapshot_token` @@ -743,7 +744,9 @@ async def get_membership_for_user_at_to_token( event_pos=first_membership_change_after_to_token.prev_event_pos, membership=first_membership_change_after_to_token.prev_membership, sender=first_membership_change_after_to_token.prev_sender, + # We will update these fields below to be accurate newly_joined=False, + newly_left=False, is_dm=False, ) else: @@ -797,7 +800,9 @@ async def get_membership_for_user_at_to_token( if membership_change.membership != Membership.JOIN: has_non_join_event_by_room_id_in_from_to_range[room_id] = True - # 2) We also want to assemble a list of possibly newly joined rooms. Someone + # 2) Fixup + # + # 3) We also want to assemble a list of possibly newly joined rooms. Someone # could have left and joined multiple times during the given range but we only # care about whether they are joined at the end of the token range so we are # working with the last membership even in the token range. @@ -807,11 +812,19 @@ async def get_membership_for_user_at_to_token( ) in last_membership_change_by_room_id_in_from_to_range.values(): room_id = last_membership_change_in_from_to_range.room_id - # 2) + # 3) if last_membership_change_in_from_to_range.membership == Membership.JOIN: possibly_newly_joined_room_ids.add(room_id) - # 2) Figure out `newly_joined` + # 2) Add back newly_left rooms (> `from_token` and <= `to_token`). We + # include newly_left rooms because the last event that the user should see + # is their own leave event + if last_membership_change_in_from_to_range.membership == Membership.LEAVE: + sync_room_id_set[room_id] = sync_room_id_set[room_id].copy_and_replace( + newly_left=True + ) + + # 3) Figure out `newly_joined` for room_id in possibly_newly_joined_room_ids: has_non_join_in_from_to_range = ( has_non_join_event_by_room_id_in_from_to_range.get(room_id, False) @@ -846,7 +859,7 @@ async def get_membership_for_user_at_to_token( room_id ].copy_and_replace(newly_joined=True) - # 3) Figure out which rooms the user considers to be direct-message (DM) rooms + # 4) Figure out which rooms the user considers to be direct-message (DM) rooms # # We're using global account data (`m.direct`) instead of checking for # `is_direct` on membership events because that property only appears for @@ -869,7 +882,7 @@ async def get_membership_for_user_at_to_token( if isinstance(room_id, str): dm_room_id_set.add(room_id) - # 3) Fixup + # 4) Fixup for room_id in sync_room_id_set: sync_room_id_set[room_id] = sync_room_id_set[room_id].copy_and_replace( is_dm=room_id in dm_room_id_set @@ -881,7 +894,7 @@ async def get_sync_room_ids_for_user( self, user: UserID, to_token: StreamToken, - from_token: Optional[StreamToken] = None, + from_token: Optional[StreamToken], ) -> Dict[str, _RoomMembershipForUser]: """ Fetch room IDs that should be listed for this user in the sync response (the @@ -914,6 +927,7 @@ async def get_sync_room_ids_for_user( sync_room_id_set = await self.get_membership_for_user_at_to_token( user=user, to_token=to_token, + from_token=from_token, ) # Filter the rooms @@ -955,15 +969,17 @@ async def check_room_subscription_allowed_for_user( room_id=room_id, ) if membership is not None and member_event_id is not None: - return _RoomMembershipForUser( - room_id=room_id, - event_id=member_event_id, - event_pos=room_for_user.event_pos, - membership=membership, - sender=room_for_user.sender, - newly_joined=False, - is_dm=False, - ) + return None + # return _RoomMembershipForUser( + # room_id=room_id, + # event_id=member_event_id, + # event_pos=room_for_user.event_pos, + # membership=membership, + # sender=room_for_user.sender, + # newly_joined=False, + # newly_left=False, + # is_dm=False, + # ) # If the room is `world_readable`, it doesn't matter whether they can join, # everyone can see the room. @@ -975,15 +991,19 @@ async def check_room_subscription_allowed_for_user( and visibility.content.get("history_visibility") == HistoryVisibility.WORLD_READABLE ): - return _RoomMembershipForUser( - room_id=room_id, - event_id=None, - event_pos=None, - membership=None, - sender=None, - newly_joined=False, - is_dm=False, - ) + return None + # return _RoomMembershipForUser( + # room_id=room_id, + # event_id=None, + # event_pos=None, + # membership=None, + # sender=None, + # newly_joined=False, + # newly_left=False, + # is_dm=False, + # ) + + return None async def filter_rooms( self, diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 9dd2363adc..f9d488655d 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -943,13 +943,9 @@ def test_only_newly_left_rooms_show_up(self) -> None: # Only the newly_left room should show up self.assertEqual(room_id_results.keys(), {room_id2}) - # It should be pointing to the latest membership event in the from/to range but - # the `event_id` is `None` because we left the room causing the server to leave - # the room because no other local users are in it (quirk of the - # `current_state_delta_stream` table that we source things from) self.assertEqual( room_id_results[room_id2].event_id, - None, # _leave_response2["event_id"], + _leave_response2["event_id"], ) # We should *NOT* be `newly_joined` because we are instead `newly_left` self.assertEqual(room_id_results[room_id2].newly_joined, False) @@ -990,7 +986,9 @@ def test_no_joins_after_to_token(self) -> None: join_response1["event_id"], ) # We should be `newly_joined` because we joined during the token range - self.assertEqual(room_id_results[room_id1].newly_joined, True) + self.assertEqual( + room_id_results[room_id1].newly_joined, True, room_id_results[room_id1] + ) def test_join_during_range_and_left_room_after_to_token(self) -> None: """ From de1b493fdb10bb23a8a55f8d6477261d20a320cf Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 10 Jul 2024 17:28:28 -0500 Subject: [PATCH 21/39] Catch state resets --- synapse/handlers/sliding_sync.py | 43 +++++++++--- tests/handlers/test_sliding_sync.py | 101 ++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 8 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 95371facc1..13eb5ce716 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -113,7 +113,9 @@ def filter_membership_for_sync( newly_left = room_membership_for_user.newly_left # We want to allow everything except rooms the user has left unless `newly_left` - # because we want everything that's *still* relevant to the user. + # because we want everything that's *still* relevant to the user. We include + # `newly_left` rooms because the last event that the user should see is their own + # leave event. # # A leave != kick. This logic includes kicks (leave events where the sender is not # the same user) and can be read as "anything that isn't a leave or newly_left or a @@ -123,7 +125,6 @@ def filter_membership_for_sync( # happened that removed the user from the room, or the user was the last person # locally to leave the room which caused the server to leave the room. In both # cases, we can just remove the rooms since they are no longer relevant to the user. - # They could still be added back later if they are `newly_left`. return membership != Membership.LEAVE or newly_left or sender not in (user_id, None) @@ -816,13 +817,39 @@ async def get_membership_for_user_at_to_token( if last_membership_change_in_from_to_range.membership == Membership.JOIN: possibly_newly_joined_room_ids.add(room_id) - # 2) Add back newly_left rooms (> `from_token` and <= `to_token`). We - # include newly_left rooms because the last event that the user should see - # is their own leave event + # 2) Figure out newly_left rooms (> `from_token` and <= `to_token`). if last_membership_change_in_from_to_range.membership == Membership.LEAVE: - sync_room_id_set[room_id] = sync_room_id_set[room_id].copy_and_replace( - newly_left=True - ) + # 2) Mark this room as `newly_left` + + # If we're seeing a membership change here, we should expect to already + # have it in our snapshot but if a state reset happens, it wouldn't have + # shown up in our snapshot but appear as a change here. + existing_sync_entry = sync_room_id_set.get(room_id) + if existing_sync_entry is not None: + # Normal expected case + sync_room_id_set[room_id] = existing_sync_entry.copy_and_replace( + newly_left=True + ) + else: + # State reset! + logger.warn( + "State reset detected for room_id %s with %s who is no longer in the room", + room_id, + user_id, + ) + # Even though a state reset happened which removed the person from + # the room, we still add it the list so the user knows they left the + # room. + sync_room_id_set[room_id] = _RoomMembershipForUser( + room_id=room_id, + event_id=last_membership_change_in_from_to_range.event_id, + event_pos=last_membership_change_in_from_to_range.event_pos, + membership=last_membership_change_in_from_to_range.membership, + sender=last_membership_change_in_from_to_range.sender, + newly_joined=False, + newly_left=True, + is_dm=False, + ) # 3) Figure out `newly_joined` for room_id in possibly_newly_joined_room_ids: diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index f9d488655d..629eeaaf93 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -2123,6 +2123,107 @@ def test_multiple_rooms_are_not_confused( }, ) + def test_state_reset(self) -> None: + """ + Test a state reset scenario where the user gets removed from the room (when + there is no corresponding leave event) + """ + 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") + + # The room where the state reset will happen + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + join_response1 = self.helper.join(room_id1, user1_id, tok=user1_tok) + + # Join another room so we don't hit the short-circuit and return early if they + # have no room membership + room_id2 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.join(room_id2, user1_id, tok=user1_tok) + + before_reset_token = self.event_sources.get_current_token() + + # Send another state event to make a position for the state reset to happen at + dummy_state_response = self.helper.send_state( + room_id1, + event_type="foobarbaz", + state_key="", + body={"foo": "bar"}, + tok=user2_tok, + ) + dummy_state_pos = self.get_success( + self.store.get_position_for_event(dummy_state_response["event_id"]) + ) + + # Mock a state reset removing the membership for user1 in the current state + self.get_success( + self.store.db_pool.simple_delete( + table="current_state_events", + keyvalues={ + "room_id": room_id1, + "type": EventTypes.Member, + "state_key": user1_id, + }, + desc="state reset user in current_state_events", + ) + ) + self.get_success( + self.store.db_pool.simple_delete( + table="local_current_membership", + keyvalues={ + "room_id": room_id1, + "user_id": user1_id, + }, + desc="state reset user in local_current_membership", + ) + ) + self.get_success( + self.store.db_pool.simple_insert( + table="current_state_delta_stream", + values={ + "stream_id": dummy_state_pos.stream, + "room_id": room_id1, + "type": EventTypes.Member, + "state_key": user1_id, + "event_id": None, + "prev_event_id": join_response1["event_id"], + "instance_name": dummy_state_pos.instance_name, + }, + desc="state reset user in current_state_delta_stream", + ) + ) + + # Manually bust the cache since we we're just manually messing with the database + # and not causing an actual state reset. + self.store._membership_stream_cache.entity_has_changed( + user1_id, dummy_state_pos.stream + ) + + after_reset_token = self.event_sources.get_current_token() + + # The function under test + room_id_results = self.get_success( + self.sliding_sync_handler.get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=before_reset_token, + to_token=after_reset_token, + ) + ) + + # Room1 should show up because it was `newly_left` via state reset during the from/to range + self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) + # It should be pointing to no event because we were removed from the room + # without a corresponding leave event + self.assertEqual( + room_id_results[room_id1].event_id, + None, + ) + # We should *NOT* be `newly_joined` because we joined before the token range + self.assertEqual(room_id_results[room_id1].newly_joined, False) + # We should be `newly_left` because we were removed via state reset during the from/to range + self.assertEqual(room_id_results[room_id1].newly_left, True) + class GetSyncRoomIdsForUserEventShardTestCase(BaseMultiWorkerStreamTestCase): """ From 27df0ed51a54b3f702695d3b6661eced257b1bf7 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 10 Jul 2024 17:38:10 -0500 Subject: [PATCH 22/39] Explain state reset handling --- synapse/handlers/sliding_sync.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 13eb5ce716..4684c9de43 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -121,10 +121,10 @@ def filter_membership_for_sync( # the same user) and can be read as "anything that isn't a leave or newly_left or a # leave with a different sender". # - # When `sender=None` and `membership=Membership.LEAVE`, it means that a state reset - # happened that removed the user from the room, or the user was the last person - # locally to leave the room which caused the server to leave the room. In both - # cases, we can just remove the rooms since they are no longer relevant to the user. + # When `sender=None`, it means that a state reset happened that removed the user + # from the room without a corresponding leave event. We can just remove the rooms + # since they are no longer relevant to the user but will still appear if they are + # `newly_left`. return membership != Membership.LEAVE or newly_left or sender not in (user_id, None) @@ -839,7 +839,8 @@ async def get_membership_for_user_at_to_token( ) # Even though a state reset happened which removed the person from # the room, we still add it the list so the user knows they left the - # room. + # room. Downstream code can check for a state reset by looking for + # `event_id=None and membership is not None`. sync_room_id_set[room_id] = _RoomMembershipForUser( room_id=room_id, event_id=last_membership_change_in_from_to_range.event_id, @@ -1494,10 +1495,10 @@ async def get_room_sync_data( stripped_state.append(strip_event(invite_or_knock_event)) # TODO: Handle state resets. For example, if we see - # `room_membership_for_user_at_to_token.membership = Membership.LEAVE` but - # `required_state` doesn't include it, we should indicate to the client that a - # state reset happened. Perhaps we should indicate this by setting `initial: - # True` and empty `required_state`. + # `room_membership_for_user_at_to_token.event_id=None and + # room_membership_for_user_at_to_token.membership is not None`, we should + # indicate to the client that a state reset happened. Perhaps we should indicate + # this by setting `initial: True` and empty `required_state`. # TODO: Since we can't determine whether we've already sent a room down this # Sliding Sync connection before (we plan to add this optimization in the From 0b8ee4e62bc54cf51769638cb8909e69d29eb281 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 10 Jul 2024 19:21:43 -0500 Subject: [PATCH 23/39] Test `get_room_membership_for_user_at_to_token()` --- synapse/handlers/sliding_sync.py | 4 +- tests/handlers/test_sliding_sync.py | 434 ++++++++++++++++++++-------- tests/rest/client/test_sync.py | 54 +--- tests/unittest.py | 51 ++++ 4 files changed, 366 insertions(+), 177 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 4684c9de43..c91fdca6bf 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -587,7 +587,7 @@ async def current_sync_for_user( extensions={}, ) - async def get_membership_for_user_at_to_token( + async def get_room_membership_for_user_at_to_token( self, user: UserID, to_token: StreamToken, @@ -952,7 +952,7 @@ async def get_sync_room_ids_for_user( """ user_id = user.to_string() - sync_room_id_set = await self.get_membership_for_user_at_to_token( + sync_room_id_set = await self.get_room_membership_for_user_at_to_token( user=user, to_token=to_token, from_token=from_token, diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 629eeaaf93..fa6e75e10f 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -579,9 +579,9 @@ def test_combine_room_sync_config( self._assert_room_config_equal(room_sync_config_b, expected, "A into B") -class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): +class GetRoomMembershipForUserAtToTokenTestCase(HomeserverTestCase): """ - Tests Sliding Sync handler `get_sync_room_ids_for_user()` to make sure it returns + Tests Sliding Sync handler `get_room_membership_for_user_at_to_token()` to make sure it returns the correct list of rooms IDs. """ @@ -614,7 +614,7 @@ def test_no_rooms(self) -> None: now_token = self.event_sources.get_current_token() room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=now_token, to_token=now_token, @@ -641,7 +641,7 @@ def test_get_newly_joined_room(self) -> None: after_room_token = self.event_sources.get_current_token() room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room_token, to_token=after_room_token, @@ -655,9 +655,11 @@ def test_get_newly_joined_room(self) -> None: room_id_results[room_id].event_id, join_response["event_id"], ) + self.assertEqual(room_id_results[room_id].membership, Membership.JOIN) # We should be considered `newly_joined` because we joined during the token # range self.assertEqual(room_id_results[room_id].newly_joined, True) + self.assertEqual(room_id_results[room_id].newly_left, False) def test_get_already_joined_room(self) -> None: """ @@ -674,7 +676,7 @@ def test_get_already_joined_room(self) -> None: after_room_token = self.event_sources.get_current_token() room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room_token, to_token=after_room_token, @@ -688,8 +690,10 @@ def test_get_already_joined_room(self) -> None: room_id_results[room_id].event_id, join_response["event_id"], ) + self.assertEqual(room_id_results[room_id].membership, Membership.JOIN) # We should *NOT* be `newly_joined` because we joined before the token range self.assertEqual(room_id_results[room_id].newly_joined, False) + self.assertEqual(room_id_results[room_id].newly_left, False) def test_get_invited_banned_knocked_room(self) -> None: """ @@ -746,7 +750,7 @@ def test_get_invited_banned_knocked_room(self) -> None: after_room_token = self.event_sources.get_current_token() room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room_token, to_token=after_room_token, @@ -768,19 +772,25 @@ def test_get_invited_banned_knocked_room(self) -> None: room_id_results[invited_room_id].event_id, invite_response["event_id"], ) + self.assertEqual(room_id_results[invited_room_id].membership, Membership.INVITE) + self.assertEqual(room_id_results[invited_room_id].newly_joined, False) + self.assertEqual(room_id_results[invited_room_id].newly_left, False) + self.assertEqual( room_id_results[ban_room_id].event_id, ban_response["event_id"], ) + self.assertEqual(room_id_results[ban_room_id].membership, Membership.BAN) + self.assertEqual(room_id_results[ban_room_id].newly_joined, False) + self.assertEqual(room_id_results[ban_room_id].newly_left, False) + self.assertEqual( room_id_results[knock_room_id].event_id, knock_room_membership_state_event.event_id, ) - # We should *NOT* be `newly_joined` because we were not joined at the the time - # of the `to_token`. - self.assertEqual(room_id_results[invited_room_id].newly_joined, False) - self.assertEqual(room_id_results[ban_room_id].newly_joined, False) + self.assertEqual(room_id_results[knock_room_id].membership, Membership.KNOCK) self.assertEqual(room_id_results[knock_room_id].newly_joined, False) + self.assertEqual(room_id_results[knock_room_id].newly_left, False) def test_get_kicked_room(self) -> None: """ @@ -812,7 +822,7 @@ def test_get_kicked_room(self) -> None: after_kick_token = self.event_sources.get_current_token() room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_kick_token, to_token=after_kick_token, @@ -826,9 +836,12 @@ def test_get_kicked_room(self) -> None: room_id_results[kick_room_id].event_id, kick_response["event_id"], ) + self.assertEqual(room_id_results[kick_room_id].membership, Membership.LEAVE) + self.assertNotEqual(room_id_results[kick_room_id].sender, user1_id) # We should *NOT* be `newly_joined` because we were not joined at the the time # of the `to_token`. self.assertEqual(room_id_results[kick_room_id].newly_joined, False) + self.assertEqual(room_id_results[kick_room_id].newly_left, False) def test_forgotten_rooms(self) -> None: """ @@ -902,7 +915,7 @@ def test_forgotten_rooms(self) -> None: self.assertEqual(channel.code, 200, channel.result) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room_forgets, to_token=before_room_forgets, @@ -912,48 +925,58 @@ def test_forgotten_rooms(self) -> None: # We shouldn't see the room because it was forgotten self.assertEqual(room_id_results.keys(), set()) - def test_only_newly_left_rooms_show_up(self) -> None: + def test_newly_left_rooms(self) -> None: """ - Test that newly_left rooms still show up in the sync response but rooms that - were left before the `from_token` don't show up. See condition "2)" comments in - the `get_sync_room_ids_for_user` method. + Test that newly_left are marked properly """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") # Leave before we calculate the `from_token` room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok) - self.helper.leave(room_id1, user1_id, tok=user1_tok) + leave_response1 = self.helper.leave(room_id1, user1_id, tok=user1_tok) after_room1_token = self.event_sources.get_current_token() # Leave during the from_token/to_token range (newly_left) room_id2 = self.helper.create_room_as(user1_id, tok=user1_tok) - _leave_response2 = self.helper.leave(room_id2, user1_id, tok=user1_tok) + leave_response2 = self.helper.leave(room_id2, user1_id, tok=user1_tok) after_room2_token = self.event_sources.get_current_token() room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room1_token, to_token=after_room2_token, ) ) - # Only the newly_left room should show up - self.assertEqual(room_id_results.keys(), {room_id2}) + self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) + + self.assertEqual( + room_id_results[room_id1].event_id, + leave_response1["event_id"], + ) + self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + # We should *NOT* be `newly_joined` or `newly_left` because that happened before + # the from/to range + self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) + self.assertEqual( room_id_results[room_id2].event_id, - _leave_response2["event_id"], + leave_response2["event_id"], ) + self.assertEqual(room_id_results[room_id2].membership, Membership.LEAVE) # We should *NOT* be `newly_joined` because we are instead `newly_left` self.assertEqual(room_id_results[room_id2].newly_joined, False) + self.assertEqual(room_id_results[room_id2].newly_left, True) def test_no_joins_after_to_token(self) -> None: """ Rooms we join after the `to_token` should *not* show up. See condition "1b)" - comments in the `get_sync_room_ids_for_user()` method. + comments in the `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -972,7 +995,7 @@ def test_no_joins_after_to_token(self) -> None: self.helper.join(room_id2, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room1_token, to_token=after_room1_token, @@ -985,16 +1008,16 @@ def test_no_joins_after_to_token(self) -> None: room_id_results[room_id1].event_id, join_response1["event_id"], ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should be `newly_joined` because we joined during the token range - self.assertEqual( - room_id_results[room_id1].newly_joined, True, room_id_results[room_id1] - ) + self.assertEqual(room_id_results[room_id1].newly_joined, True) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_join_during_range_and_left_room_after_to_token(self) -> None: """ Room still shows up if we left the room but were joined during the from_token/to_token. See condition "1a)" comments in the - `get_sync_room_ids_for_user()` method. + `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1012,7 +1035,7 @@ def test_join_during_range_and_left_room_after_to_token(self) -> None: leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room1_token, to_token=after_room1_token, @@ -1034,14 +1057,16 @@ def test_join_during_range_and_left_room_after_to_token(self) -> None: } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should be `newly_joined` because we joined during the token range self.assertEqual(room_id_results[room_id1].newly_joined, True) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_join_before_range_and_left_room_after_to_token(self) -> None: """ Room still shows up if we left the room but were joined before the `from_token` so it should show up. See condition "1a)" comments in the - `get_sync_room_ids_for_user()` method. + `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1057,7 +1082,7 @@ def test_join_before_range_and_left_room_after_to_token(self) -> None: leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room1_token, to_token=after_room1_token, @@ -1078,14 +1103,16 @@ def test_join_before_range_and_left_room_after_to_token(self) -> None: } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should *NOT* be `newly_joined` because we joined before the token range self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_kicked_before_range_and_left_after_to_token(self) -> None: """ Room still shows up if we left the room but were kicked before the `from_token` so it should show up. See condition "1a)" comments in the - `get_sync_room_ids_for_user()` method. + `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1119,7 +1146,7 @@ def test_kicked_before_range_and_left_after_to_token(self) -> None: leave_response = self.helper.leave(kick_room_id, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_kick_token, to_token=after_kick_token, @@ -1142,14 +1169,17 @@ def test_kicked_before_range_and_left_after_to_token(self) -> None: } ), ) + self.assertEqual(room_id_results[kick_room_id].membership, Membership.LEAVE) + self.assertNotEqual(room_id_results[kick_room_id].sender, user1_id) # We should *NOT* be `newly_joined` because we were kicked self.assertEqual(room_id_results[kick_room_id].newly_joined, False) + self.assertEqual(room_id_results[kick_room_id].newly_left, False) def test_newly_left_during_range_and_join_leave_after_to_token(self) -> None: """ Newly left room should show up. But we're also testing that joining and leaving after the `to_token` doesn't mess with the results. See condition "2)" and "1a)" - comments in the `get_sync_room_ids_for_user()` method. + comments in the `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1172,7 +1202,7 @@ def test_newly_left_during_range_and_join_leave_after_to_token(self) -> None: leave_response2 = self.helper.leave(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room1_token, to_token=after_room1_token, @@ -1195,14 +1225,17 @@ def test_newly_left_during_range_and_join_leave_after_to_token(self) -> None: } ), ) - # We should *NOT* be `newly_joined` because we left during the token range + self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + # We should *NOT* be `newly_joined` because we are actually `newly_left` during + # the token range self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, True) def test_newly_left_during_range_and_join_after_to_token(self) -> None: """ Newly left room should show up. But we're also testing that joining after the `to_token` doesn't mess with the results. See condition "2)" and "1b)" comments - in the `get_sync_room_ids_for_user()` method. + in the `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1224,7 +1257,7 @@ def test_newly_left_during_range_and_join_after_to_token(self) -> None: join_response2 = self.helper.join(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room1_token, to_token=after_room1_token, @@ -1246,16 +1279,19 @@ def test_newly_left_during_range_and_join_after_to_token(self) -> None: } ), ) - # We should *NOT* be `newly_joined` because we left during the token range + self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + # We should *NOT* be `newly_joined` because we are actually `newly_left` during + # the token range self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, True) def test_no_from_token(self) -> None: """ - Test that if we don't provide a `from_token`, we get all the rooms that we we're - joined up to the `to_token`. + Test that if we don't provide a `from_token`, we get all the rooms that we had + membership in up to the `to_token`. - Providing `from_token` only really has the effect that it adds `newly_left` - rooms to the response. + Providing `from_token` only really has the effect that it marks rooms as + `newly_left` in the response. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1272,7 +1308,7 @@ def test_no_from_token(self) -> None: # Join and leave the room2 before the `to_token` self.helper.join(room_id2, user1_id, tok=user1_tok) - self.helper.leave(room_id2, user1_id, tok=user1_tok) + leave_response2 = self.helper.leave(room_id2, user1_id, tok=user1_tok) after_room1_token = self.event_sources.get_current_token() @@ -1280,7 +1316,7 @@ def test_no_from_token(self) -> None: self.helper.join(room_id2, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=None, to_token=after_room1_token, @@ -1288,15 +1324,31 @@ def test_no_from_token(self) -> None: ) # Only rooms we were joined to before the `to_token` should show up - self.assertEqual(room_id_results.keys(), {room_id1}) + self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) + + # Room1 # It should be pointing to the latest membership event in the from/to range self.assertEqual( room_id_results[room_id1].event_id, join_response1["event_id"], ) - # We should *NOT* be `newly_joined` because there is no `from_token` to - # define a "live" range to compare against + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + # We should *NOT* be `newly_joined`/`newly_left` because there is no + # `from_token` to define a "live" range to compare against self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) + + # Room2 + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id2].event_id, + leave_response2["event_id"], + ) + self.assertEqual(room_id_results[room_id2].membership, Membership.LEAVE) + # We should *NOT* be `newly_joined`/`newly_left` because there is no + # `from_token` to define a "live" range to compare against + self.assertEqual(room_id_results[room_id2].newly_joined, False) + self.assertEqual(room_id_results[room_id2].newly_left, False) def test_from_token_ahead_of_to_token(self) -> None: """ @@ -1315,28 +1367,28 @@ def test_from_token_ahead_of_to_token(self) -> None: room_id3 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) room_id4 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) - # Join room1 before `before_room_token` - join_response1 = self.helper.join(room_id1, user1_id, tok=user1_tok) + # Join room1 before `to_token` + join_room1_response1 = self.helper.join(room_id1, user1_id, tok=user1_tok) - # Join and leave the room2 before `before_room_token` - self.helper.join(room_id2, user1_id, tok=user1_tok) - self.helper.leave(room_id2, user1_id, tok=user1_tok) + # Join and leave the room2 before `to_token` + _join_room2_response1 = self.helper.join(room_id2, user1_id, tok=user1_tok) + leave_room2_response1 = self.helper.leave(room_id2, user1_id, tok=user1_tok) # Note: These are purposely swapped. The `from_token` should come after # the `to_token` in this test to_token = self.event_sources.get_current_token() - # Join room2 after `before_room_token` - self.helper.join(room_id2, user1_id, tok=user1_tok) + # Join room2 after `to_token` + _join_room2_response2 = self.helper.join(room_id2, user1_id, tok=user1_tok) # -------- - # Join room3 after `before_room_token` - self.helper.join(room_id3, user1_id, tok=user1_tok) + # Join room3 after `to_token` + _join_room3_response1 = self.helper.join(room_id3, user1_id, tok=user1_tok) - # Join and leave the room4 after `before_room_token` - self.helper.join(room_id4, user1_id, tok=user1_tok) - self.helper.leave(room_id4, user1_id, tok=user1_tok) + # Join and leave the room4 after `to_token` + _join_room4_response1 = self.helper.join(room_id4, user1_id, tok=user1_tok) + _leave_room4_response1 = self.helper.leave(room_id4, user1_id, tok=user1_tok) # Note: These are purposely swapped. The `from_token` should come after the # `to_token` in this test @@ -1346,31 +1398,59 @@ def test_from_token_ahead_of_to_token(self) -> None: self.helper.join(room_id4, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=from_token, to_token=to_token, ) ) - # Only rooms we were joined to before the `to_token` should show up - # - # There won't be any newly_left rooms because the `from_token` is ahead of the - # `to_token` and that range will give no membership changes to check. - self.assertEqual(room_id_results.keys(), {room_id1}) + # In the "current" state snapshot, we're joined to all of the rooms but in the + # from/to token range... + self.assertIncludes( + room_id_results.keys(), + { + # Included because we were joined before both tokens + room_id1, + # Included because we had membership before the to_token + room_id2, + # Excluded because we joined after the `to_token` + # room_id3, + # Excluded because we joined after the `to_token` + # room_id4, + }, + exact=True, + ) + + # Room1 # It should be pointing to the latest membership event in the from/to range self.assertEqual( room_id_results[room_id1].event_id, - join_response1["event_id"], + join_room1_response1["event_id"], ) - # We should *NOT* be `newly_joined` because we joined `room1` before either of the tokens + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + # We should *NOT* be `newly_joined`/`newly_left` because we joined `room1` + # before either of the tokens self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) + + # Room2 + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id2].event_id, + leave_room2_response1["event_id"], + ) + self.assertEqual(room_id_results[room_id2].membership, Membership.LEAVE) + # We should *NOT* be `newly_joined`/`newly_left` because we joined and left + # `room1` before either of the tokens + self.assertEqual(room_id_results[room_id2].newly_joined, False) + self.assertEqual(room_id_results[room_id2].newly_left, False) def test_leave_before_range_and_join_leave_after_to_token(self) -> None: """ - Old left room shouldn't show up. But we're also testing that joining and leaving - after the `to_token` doesn't mess with the results. See condition "1a)" comments - in the `get_sync_room_ids_for_user()` method. + Test old left rooms. But we're also testing that joining and leaving after the + `to_token` doesn't mess with the results. See condition "1a)" comments in the + `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1382,7 +1462,7 @@ def test_leave_before_range_and_join_leave_after_to_token(self) -> None: room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) # Join and leave the room before the from/to range self.helper.join(room_id1, user1_id, tok=user1_tok) - self.helper.leave(room_id1, user1_id, tok=user1_tok) + leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) after_room1_token = self.event_sources.get_current_token() @@ -1391,21 +1471,30 @@ def test_leave_before_range_and_join_leave_after_to_token(self) -> None: self.helper.leave(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room1_token, to_token=after_room1_token, ) ) - # Room shouldn't show up because it was left before the `from_token` - self.assertEqual(room_id_results.keys(), set()) + self.assertEqual(room_id_results.keys(), {room_id1}) + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id1].event_id, + leave_response["event_id"], + ) + self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + # We should *NOT* be `newly_joined`/`newly_left` because we joined and left + # `room1` before either of the tokens + self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_leave_before_range_and_join_after_to_token(self) -> None: """ - Old left room shouldn't show up. But we're also testing that joining after the - `to_token` doesn't mess with the results. See condition "1b)" comments in the - `get_sync_room_ids_for_user()` method. + Test old left room. But we're also testing that joining after the `to_token` + doesn't mess with the results. See condition "1b)" comments in the + `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1417,7 +1506,7 @@ def test_leave_before_range_and_join_after_to_token(self) -> None: room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) # Join and leave the room before the from/to range self.helper.join(room_id1, user1_id, tok=user1_tok) - self.helper.leave(room_id1, user1_id, tok=user1_tok) + leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) after_room1_token = self.event_sources.get_current_token() @@ -1425,24 +1514,32 @@ def test_leave_before_range_and_join_after_to_token(self) -> None: self.helper.join(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room1_token, to_token=after_room1_token, ) ) - # Room shouldn't show up because it was left before the `from_token` - self.assertEqual(room_id_results.keys(), set()) + self.assertEqual(room_id_results.keys(), {room_id1}) + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id1].event_id, + leave_response["event_id"], + ) + self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + # We should *NOT* be `newly_joined`/`newly_left` because we joined and left + # `room1` before either of the tokens + self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_join_leave_multiple_times_during_range_and_after_to_token( self, ) -> None: """ Join and leave multiple times shouldn't affect rooms from showing up. It just - matters that we were joined or newly_left in the from/to range. But we're also - testing that joining and leaving after the `to_token` doesn't mess with the - results. + matters that we had membership in the from/to range. But we're also testing that + joining and leaving after the `to_token` doesn't mess with the results. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1454,7 +1551,7 @@ def test_join_leave_multiple_times_during_range_and_after_to_token( # We create the room with user2 so the room isn't left with no members when we # leave and can still re-join. room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) - # Join, leave, join back to the room before the from/to range + # Join, leave, join back to the room during the from/to range join_response1 = self.helper.join(room_id1, user1_id, tok=user1_tok) leave_response1 = self.helper.leave(room_id1, user1_id, tok=user1_tok) join_response2 = self.helper.join(room_id1, user1_id, tok=user1_tok) @@ -1467,7 +1564,7 @@ def test_join_leave_multiple_times_during_range_and_after_to_token( leave_response3 = self.helper.leave(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room1_token, to_token=after_room1_token, @@ -1492,15 +1589,19 @@ def test_join_leave_multiple_times_during_range_and_after_to_token( } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should be `newly_joined` because we joined during the token range self.assertEqual(room_id_results[room_id1].newly_joined, True) + # We should *NOT* be `newly_left` because we joined during the token range and + # was still joined at the end of the range + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_join_leave_multiple_times_before_range_and_after_to_token( self, ) -> None: """ Join and leave multiple times before the from/to range shouldn't affect rooms - from showing up. It just matters that we were joined or newly_left in the + from showing up. It just matters that we had membership in the from/to range. But we're also testing that joining and leaving after the `to_token` doesn't mess with the results. """ @@ -1525,7 +1626,7 @@ def test_join_leave_multiple_times_before_range_and_after_to_token( leave_response3 = self.helper.leave(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room1_token, to_token=after_room1_token, @@ -1550,8 +1651,10 @@ def test_join_leave_multiple_times_before_range_and_after_to_token( } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should *NOT* be `newly_joined` because we joined before the token range self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_invite_before_range_and_join_leave_after_to_token( self, @@ -1559,7 +1662,7 @@ def test_invite_before_range_and_join_leave_after_to_token( """ Make it look like we joined after the token range but we were invited before the from/to range so the room should still show up. See condition "1a)" comments in - the `get_sync_room_ids_for_user()` method. + the `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1582,7 +1685,7 @@ def test_invite_before_range_and_join_leave_after_to_token( leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room1_token, to_token=after_room1_token, @@ -1604,9 +1707,11 @@ def test_invite_before_range_and_join_leave_after_to_token( } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.INVITE) # We should *NOT* be `newly_joined` because we were only invited before the # token range self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_join_and_display_name_changes_in_token_range( self, @@ -1654,7 +1759,7 @@ def test_join_and_display_name_changes_in_token_range( ) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room1_token, to_token=after_room1_token, @@ -1680,8 +1785,10 @@ def test_join_and_display_name_changes_in_token_range( } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should be `newly_joined` because we joined during the token range self.assertEqual(room_id_results[room_id1].newly_joined, True) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_display_name_changes_in_token_range( self, @@ -1717,7 +1824,7 @@ def test_display_name_changes_in_token_range( after_change1_token = self.event_sources.get_current_token() room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room1_token, to_token=after_change1_token, @@ -1740,8 +1847,10 @@ def test_display_name_changes_in_token_range( } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should *NOT* be `newly_joined` because we joined before the token range self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_display_name_changes_before_and_after_token_range( self, @@ -1787,7 +1896,7 @@ def test_display_name_changes_before_and_after_token_range( ) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room1_token, to_token=after_room1_token, @@ -1813,8 +1922,10 @@ def test_display_name_changes_before_and_after_token_range( } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should *NOT* be `newly_joined` because we joined before the token range self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_display_name_changes_leave_after_token_range( self, @@ -1824,7 +1935,7 @@ def test_display_name_changes_leave_after_token_range( if there are multiple `join` membership events in a row indicating `displayname`/`avatar_url` updates and we leave after the `to_token`. - See condition "1a)" comments in the `get_sync_room_ids_for_user()` method. + See condition "1a)" comments in the `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1867,7 +1978,7 @@ def test_display_name_changes_leave_after_token_range( self.helper.leave(room_id1, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room1_token, to_token=after_room1_token, @@ -1893,8 +2004,10 @@ def test_display_name_changes_leave_after_token_range( } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should be `newly_joined` because we joined during the token range self.assertEqual(room_id_results[room_id1].newly_joined, True) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_display_name_changes_join_after_token_range( self, @@ -1904,7 +2017,7 @@ def test_display_name_changes_join_after_token_range( indicating `displayname`/`avatar_url` updates doesn't affect the results (we joined after the token range so it shouldn't show up) - See condition "1b)" comments in the `get_sync_room_ids_for_user()` method. + See condition "1b)" comments in the `get_room_membership_for_user_at_to_token()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -1933,7 +2046,7 @@ def test_display_name_changes_join_after_token_range( ) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room1_token, to_token=after_room1_token, @@ -1969,7 +2082,7 @@ def test_newly_joined_with_leave_join_in_token_range( after_more_changes_token = self.event_sources.get_current_token() room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=after_room1_token, to_token=after_more_changes_token, @@ -1983,9 +2096,11 @@ def test_newly_joined_with_leave_join_in_token_range( room_id_results[room_id1].event_id, join_response2["event_id"], ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should be considered `newly_joined` because there is some non-join event in # between our latest join event. self.assertEqual(room_id_results[room_id1].newly_joined, True) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_newly_joined_only_joins_during_token_range( self, @@ -2032,7 +2147,7 @@ def test_newly_joined_only_joins_during_token_range( after_room1_token = self.event_sources.get_current_token() room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room1_token, to_token=after_room1_token, @@ -2058,8 +2173,10 @@ def test_newly_joined_only_joins_during_token_range( } ), ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) # We should be `newly_joined` because we first joined during the token range self.assertEqual(room_id_results[room_id1].newly_joined, True) + self.assertEqual(room_id_results[room_id1].newly_left, False) def test_multiple_rooms_are_not_confused( self, @@ -2082,16 +2199,18 @@ def test_multiple_rooms_are_not_confused( # Invited and left the room before the token self.helper.invite(room_id1, src=user2_id, targ=user1_id, tok=user2_tok) - self.helper.leave(room_id1, user1_id, tok=user1_tok) + leave_room1_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) # Invited to room2 - self.helper.invite(room_id2, src=user2_id, targ=user1_id, tok=user2_tok) + invite_room2_response = self.helper.invite( + room_id2, src=user2_id, targ=user1_id, tok=user2_tok + ) before_room3_token = self.event_sources.get_current_token() # Invited and left room3 during the from/to range room_id3 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) self.helper.invite(room_id3, src=user2_id, targ=user1_id, tok=user2_tok) - self.helper.leave(room_id3, user1_id, tok=user1_tok) + leave_room3_response = self.helper.leave(room_id3, user1_id, tok=user1_tok) after_room3_token = self.event_sources.get_current_token() @@ -2104,7 +2223,7 @@ def test_multiple_rooms_are_not_confused( self.helper.leave(room_id3, user1_id, tok=user1_tok) room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_room3_token, to_token=after_room3_token, @@ -2114,15 +2233,51 @@ def test_multiple_rooms_are_not_confused( self.assertEqual( room_id_results.keys(), { - # `room_id1` shouldn't show up because we left before the from/to range - # - # Room should show up because we were invited before the from/to range + # Left before the from/to range + room_id1, + # Invited before the from/to range room_id2, - # Room should show up because it was newly_left during the from/to range + # `newly_left` during the from/to range room_id3, }, ) + # Room1 + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id1].event_id, + leave_room1_response["event_id"], + ) + self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + # We should *NOT* be `newly_joined`/`newly_left` because we were invited and left + # before the token range + self.assertEqual(room_id_results[room_id1].newly_joined, False) + self.assertEqual(room_id_results[room_id1].newly_left, False) + + # Room2 + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id2].event_id, + invite_room2_response["event_id"], + ) + self.assertEqual(room_id_results[room_id2].membership, Membership.INVITE) + # We should *NOT* be `newly_joined`/`newly_left` because we were invited before + # the token range + self.assertEqual(room_id_results[room_id2].newly_joined, False) + self.assertEqual(room_id_results[room_id2].newly_left, False) + + # Room3 + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id3].event_id, + leave_room3_response["event_id"], + ) + self.assertEqual(room_id_results[room_id3].membership, Membership.LEAVE) + # We should be `newly_left` because we were invited and left during + # the token range + self.assertEqual(room_id_results[room_id3].newly_joined, False) + self.assertEqual(room_id_results[room_id3].newly_left, True) + def test_state_reset(self) -> None: """ Test a state reset scenario where the user gets removed from the room (when @@ -2204,7 +2359,7 @@ def test_state_reset(self) -> None: # The function under test room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_reset_token, to_token=after_reset_token, @@ -2219,15 +2374,17 @@ def test_state_reset(self) -> None: room_id_results[room_id1].event_id, None, ) + # State reset caused us to leave the room and there is no corresponding leave event + self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) # We should *NOT* be `newly_joined` because we joined before the token range self.assertEqual(room_id_results[room_id1].newly_joined, False) # We should be `newly_left` because we were removed via state reset during the from/to range self.assertEqual(room_id_results[room_id1].newly_left, True) -class GetSyncRoomIdsForUserEventShardTestCase(BaseMultiWorkerStreamTestCase): +class GetRoomMembershipForUserAtToTokenShardTestCase(BaseMultiWorkerStreamTestCase): """ - Tests Sliding Sync handler `get_sync_room_ids_for_user()` to make sure it works with + Tests Sliding Sync handler `get_room_membership_for_user_at_to_token()` to make sure it works with sharded event stream_writers enabled """ @@ -2286,7 +2443,7 @@ def test_sharded_event_persisters(self) -> None: We then send some events to advance the stream positions of worker1 and worker3 but worker2 is lagging behind because it's stuck. We are specifically testing - that `get_sync_room_ids_for_user(from_token=xxx, to_token=xxx)` should work + that `get_room_membership_for_user_at_to_token(from_token=xxx, to_token=xxx)` should work correctly in these adverse conditions. """ user1_id = self.register_user("user1", "pass") @@ -2325,7 +2482,7 @@ def test_sharded_event_persisters(self) -> None: join_response1 = self.helper.join(room_id1, user1_id, tok=user1_tok) join_response2 = self.helper.join(room_id2, user1_id, tok=user1_tok) # Leave room2 - self.helper.leave(room_id2, user1_id, tok=user1_tok) + leave_room2_response = self.helper.leave(room_id2, user1_id, tok=user1_tok) join_response3 = self.helper.join(room_id3, user1_id, tok=user1_tok) # Leave room3 self.helper.leave(room_id3, user1_id, tok=user1_tok) @@ -2362,7 +2519,7 @@ def test_sharded_event_persisters(self) -> None: # For room_id1/worker1: leave and join the room to advance the stream position # and generate membership changes. self.helper.leave(room_id1, user1_id, tok=user1_tok) - self.helper.join(room_id1, user1_id, tok=user1_tok) + join_room1_response = self.helper.join(room_id1, user1_id, tok=user1_tok) # For room_id2/worker2: which is currently stuck, join the room. join_on_worker2_response = self.helper.join(room_id2, user1_id, tok=user1_tok) # For room_id3/worker3: leave and join the room to advance the stream position @@ -2416,7 +2573,7 @@ def test_sharded_event_persisters(self) -> None: # The function under test room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( UserID.from_string(user1_id), from_token=before_stuck_activity_token, to_token=stuck_activity_token, @@ -2427,18 +2584,49 @@ def test_sharded_event_persisters(self) -> None: room_id_results.keys(), { room_id1, - # room_id2 shouldn't show up because we left before the from/to range - # and the join event during the range happened while worker2 was stuck. - # This means that from the perspective of the master, where the - # `stuck_activity_token` is generated, the stream position for worker2 - # wasn't advanced to the join yet. Looking at the `instance_map`, the - # join technically comes after `stuck_activity_token``. - # - # room_id2, + room_id2, room_id3, }, ) + # Room1 + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id1].event_id, + join_room1_response["event_id"], + ) + self.assertEqual(room_id_results[room_id1].membership, Membership.JOIN) + # We should be `newly_joined` because we joined during the token range + self.assertEqual(room_id_results[room_id1].newly_joined, True) + self.assertEqual(room_id_results[room_id1].newly_left, False) + + # Room2 + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id2].event_id, + leave_room2_response["event_id"], + ) + self.assertEqual(room_id_results[room_id2].membership, Membership.LEAVE) + # room_id2 should *NOT* be considered `newly_left` because we left before the + # from/to range and the join event during the range happened while worker2 was + # stuck. This means that from the perspective of the master, where the + # `stuck_activity_token` is generated, the stream position for worker2 wasn't + # advanced to the join yet. Looking at the `instance_map`, the join technically + # comes after `stuck_activity_token`. + self.assertEqual(room_id_results[room_id2].newly_joined, False) + self.assertEqual(room_id_results[room_id2].newly_left, False) + + # Room3 + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[room_id3].event_id, + join_on_worker3_response["event_id"], + ) + self.assertEqual(room_id_results[room_id3].membership, Membership.JOIN) + # We should be `newly_joined` because we joined during the token range + self.assertEqual(room_id_results[room_id3].newly_joined, True) + self.assertEqual(room_id_results[room_id3].newly_left, False) + class FilterRoomsTestCase(HomeserverTestCase): """ diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index bec4858acd..e2c2b90212 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -1250,7 +1250,7 @@ def _assertRequiredStateIncludes( exact: bool = False, ) -> None: """ - Wrapper around `_assertIncludes` to give slightly better looking diff error + Wrapper around `assertIncludes` to give slightly better looking diff error messages that include some context "$event_id (type, state_key)". Args: @@ -1266,7 +1266,7 @@ def _assertRequiredStateIncludes( for event in actual_required_state: assert isinstance(event, dict) - self._assertIncludes( + self.assertIncludes( { f'{event["event_id"]} ("{event["type"]}", "{event["state_key"]}")' for event in actual_required_state @@ -1280,56 +1280,6 @@ def _assertRequiredStateIncludes( message=str(actual_required_state), ) - def _assertIncludes( - self, - actual_items: AbstractSet[str], - expected_items: AbstractSet[str], - exact: bool = False, - message: Optional[str] = None, - ) -> None: - """ - Assert that all of the `expected_items` are included in the `actual_items`. - - This assert could also be called `assertContains`, `assertItemsInSet` - - Args: - actual_items: The container - expected_items: The items to check for in the container - exact: Whether the actual state should be exactly equal to the expected - state (no extras). - message: Optional message to include in the failure message. - """ - # Check that each set has the same items - if exact and actual_items == expected_items: - return - # Check for a superset - elif not exact and actual_items >= expected_items: - return - - expected_lines: List[str] = [] - for expected_item in expected_items: - is_expected_in_actual = expected_item in actual_items - expected_lines.append( - "{} {}".format(" " if is_expected_in_actual else "?", expected_item) - ) - - actual_lines: List[str] = [] - for actual_item in actual_items: - is_actual_in_expected = actual_item in expected_items - actual_lines.append( - "{} {}".format("+" if is_actual_in_expected else " ", actual_item) - ) - - newline = "\n" - expected_string = f"Expected items to be in actual ('?' = missing expected items):\n {{\n{newline.join(expected_lines)}\n }}" - actual_string = f"Actual ('+' = found expected items):\n {{\n{newline.join(actual_lines)}\n }}" - first_message = ( - "Items must match exactly" if exact else "Some expected items are missing." - ) - diff_message = f"{first_message}\n{expected_string}\n{actual_string}" - - self.fail(f"{diff_message}\n{message}") - def _add_new_dm_to_global_account_data( self, source_user_id: str, target_user_id: str, target_room_id: str ) -> None: diff --git a/tests/unittest.py b/tests/unittest.py index a7c20556a0..2b28cf6ffa 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -29,6 +29,7 @@ import time from typing import ( Any, + AbstractSet, Awaitable, Callable, ClassVar, @@ -269,6 +270,56 @@ def assert_dict(self, required: Mapping, actual: Mapping) -> None: required[key], actual[key], msg="%s mismatch. %s" % (key, actual) ) + def assertIncludes( + self, + actual_items: AbstractSet[str], + expected_items: AbstractSet[str], + exact: bool = False, + message: Optional[str] = None, + ) -> None: + """ + Assert that all of the `expected_items` are included in the `actual_items`. + + This assert could also be called `assertContains`, `assertItemsInSet` + + Args: + actual_items: The container + expected_items: The items to check for in the container + exact: Whether the actual state should be exactly equal to the expected + state (no extras). + message: Optional message to include in the failure message. + """ + # Check that each set has the same items + if exact and actual_items == expected_items: + return + # Check for a superset + elif not exact and actual_items >= expected_items: + return + + expected_lines: List[str] = [] + for expected_item in expected_items: + is_expected_in_actual = expected_item in actual_items + expected_lines.append( + "{} {}".format(" " if is_expected_in_actual else "?", expected_item) + ) + + actual_lines: List[str] = [] + for actual_item in actual_items: + is_actual_in_expected = actual_item in expected_items + actual_lines.append( + "{} {}".format("+" if is_actual_in_expected else " ", actual_item) + ) + + newline = "\n" + expected_string = f"Expected items to be in actual ('?' = missing expected items):\n {{\n{newline.join(expected_lines)}\n }}" + actual_string = f"Actual ('+' = found expected items):\n {{\n{newline.join(actual_lines)}\n }}" + first_message = ( + "Items must match exactly" if exact else "Some expected items are missing." + ) + diff_message = f"{first_message}\n{expected_string}\n{actual_string}" + + self.fail(f"{diff_message}\n{message}") + def DEBUG(target: TV) -> TV: """A decorator to set the .loglevel attribute to logging.DEBUG. From f157aaba9ec4ced66210eea629d453beccb6a2fc Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Wed, 10 Jul 2024 19:40:04 -0500 Subject: [PATCH 24/39] Add separate tests for `get_sync_room_ids_for_user()` --- synapse/handlers/sliding_sync.py | 2 +- tests/handlers/test_sliding_sync.py | 347 ++++++++++++++++++++++++++++ 2 files changed, 348 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index c91fdca6bf..5dd47ba403 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -958,7 +958,7 @@ async def get_sync_room_ids_for_user( from_token=from_token, ) - # Filter the rooms + # Filter rooms to only what we're interested to sync with filtered_sync_room_id_set = { room_id: room_membership_for_user for room_id, room_membership_for_user in sync_room_id_set.items() diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index fa6e75e10f..e8f72cf7d3 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -2628,6 +2628,353 @@ def test_sharded_event_persisters(self) -> None: self.assertEqual(room_id_results[room_id3].newly_left, False) +class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): + """ + Tests Sliding Sync handler `get_sync_room_ids_for_user()` to make sure it returns + the correct list of rooms IDs. + """ + + servlets = [ + admin.register_servlets, + knock.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def default_config(self) -> JsonDict: + config = super().default_config() + # Enable sliding sync + config["experimental_features"] = {"msc3575_enabled": True} + return config + + 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 + self.event_sources = hs.get_event_sources() + self.storage_controllers = hs.get_storage_controllers() + + def test_no_rooms(self) -> None: + """ + Test when the user has never joined any rooms before + """ + user1_id = self.register_user("user1", "pass") + # user1_tok = self.login(user1_id, "pass") + + now_token = self.event_sources.get_current_token() + + room_id_results = self.get_success( + self.sliding_sync_handler.get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=now_token, + to_token=now_token, + ) + ) + + self.assertEqual(room_id_results.keys(), set()) + + def test_basic_rooms(self) -> None: + """ + Test that rooms that the user is joined to, invited to, banned from, and knocked + on show up. + """ + 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") + + before_room_token = self.event_sources.get_current_token() + + join_room_id = self.helper.create_room_as(user2_id, tok=user2_tok) + join_response = self.helper.join(join_room_id, user1_id, tok=user1_tok) + + # Setup the invited room (user2 invites user1 to the room) + invited_room_id = self.helper.create_room_as(user2_id, tok=user2_tok) + invite_response = self.helper.invite( + invited_room_id, targ=user1_id, tok=user2_tok + ) + + # Setup the ban room (user2 bans user1 from the room) + ban_room_id = self.helper.create_room_as( + user2_id, tok=user2_tok, is_public=True + ) + self.helper.join(ban_room_id, user1_id, tok=user1_tok) + ban_response = self.helper.ban( + ban_room_id, src=user2_id, targ=user1_id, tok=user2_tok + ) + + # Setup the knock room (user1 knocks on the room) + knock_room_id = self.helper.create_room_as( + user2_id, tok=user2_tok, room_version=RoomVersions.V7.identifier + ) + self.helper.send_state( + knock_room_id, + EventTypes.JoinRules, + {"join_rule": JoinRules.KNOCK}, + tok=user2_tok, + ) + # User1 knocks on the room + knock_channel = self.make_request( + "POST", + "/_matrix/client/r0/knock/%s" % (knock_room_id,), + b"{}", + user1_tok, + ) + self.assertEqual(knock_channel.code, 200, knock_channel.result) + knock_room_membership_state_event = self.get_success( + self.storage_controllers.state.get_current_state_event( + knock_room_id, EventTypes.Member, user1_id + ) + ) + assert knock_room_membership_state_event is not None + + after_room_token = self.event_sources.get_current_token() + + room_id_results = self.get_success( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( + UserID.from_string(user1_id), + from_token=before_room_token, + to_token=after_room_token, + ) + ) + + # Ensure that the invited, ban, and knock rooms show up + self.assertEqual( + room_id_results.keys(), + { + join_room_id, + invited_room_id, + ban_room_id, + knock_room_id, + }, + ) + # It should be pointing to the the respective membership event (latest + # membership event in the from/to range) + self.assertEqual( + room_id_results[join_room_id].event_id, + join_response["event_id"], + ) + self.assertEqual(room_id_results[join_room_id].membership, Membership.JOIN) + self.assertEqual(room_id_results[join_room_id].newly_joined, True) + self.assertEqual(room_id_results[join_room_id].newly_left, False) + + self.assertEqual( + room_id_results[invited_room_id].event_id, + invite_response["event_id"], + ) + self.assertEqual(room_id_results[invited_room_id].membership, Membership.INVITE) + self.assertEqual(room_id_results[invited_room_id].newly_joined, False) + self.assertEqual(room_id_results[invited_room_id].newly_left, False) + + self.assertEqual( + room_id_results[ban_room_id].event_id, + ban_response["event_id"], + ) + self.assertEqual(room_id_results[ban_room_id].membership, Membership.BAN) + self.assertEqual(room_id_results[ban_room_id].newly_joined, False) + self.assertEqual(room_id_results[ban_room_id].newly_left, False) + + self.assertEqual( + room_id_results[knock_room_id].event_id, + knock_room_membership_state_event.event_id, + ) + self.assertEqual(room_id_results[knock_room_id].membership, Membership.KNOCK) + self.assertEqual(room_id_results[knock_room_id].newly_joined, False) + self.assertEqual(room_id_results[knock_room_id].newly_left, False) + + def test_only_newly_left_rooms_show_up(self) -> None: + """ + Test that `newly_left` rooms still show up in the sync response but rooms that + were left before the `from_token` don't show up. See condition "2)" comments in + the `get_room_membership_for_user_at_to_token()` method. + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + # Leave before we calculate the `from_token` + room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok) + self.helper.leave(room_id1, user1_id, tok=user1_tok) + + after_room1_token = self.event_sources.get_current_token() + + # Leave during the from_token/to_token range (newly_left) + room_id2 = self.helper.create_room_as(user1_id, tok=user1_tok) + _leave_response2 = self.helper.leave(room_id2, user1_id, tok=user1_tok) + + after_room2_token = self.event_sources.get_current_token() + + room_id_results = self.get_success( + self.sliding_sync_handler.get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=after_room1_token, + to_token=after_room2_token, + ) + ) + + # Only the `newly_left` room should show up + self.assertEqual(room_id_results.keys(), {room_id2}) + self.assertEqual( + room_id_results[room_id2].event_id, + _leave_response2["event_id"], + ) + # We should *NOT* be `newly_joined` because we are instead `newly_left` + self.assertEqual(room_id_results[room_id2].newly_joined, False) + self.assertEqual(room_id_results[room_id2].newly_left, True) + + def test_get_kicked_room(self) -> None: + """ + Test that a room that the user was kicked from still shows up. When the user + comes back to their client, they should see that they were kicked. + """ + 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 kick room (user2 kicks user1 from the room) + kick_room_id = self.helper.create_room_as( + user2_id, tok=user2_tok, is_public=True + ) + self.helper.join(kick_room_id, user1_id, tok=user1_tok) + # Kick user1 from the room + kick_response = self.helper.change_membership( + room=kick_room_id, + src=user2_id, + targ=user1_id, + tok=user2_tok, + membership=Membership.LEAVE, + extra_data={ + "reason": "Bad manners", + }, + ) + + after_kick_token = self.event_sources.get_current_token() + + room_id_results = self.get_success( + self.sliding_sync_handler.get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=after_kick_token, + to_token=after_kick_token, + ) + ) + + # The kicked room should show up + self.assertEqual(room_id_results.keys(), {kick_room_id}) + # It should be pointing to the latest membership event in the from/to range + self.assertEqual( + room_id_results[kick_room_id].event_id, + kick_response["event_id"], + ) + self.assertEqual(room_id_results[kick_room_id].membership, Membership.LEAVE) + self.assertNotEqual(room_id_results[kick_room_id].sender, user1_id) + # We should *NOT* be `newly_joined` because we were not joined at the the time + # of the `to_token`. + self.assertEqual(room_id_results[kick_room_id].newly_joined, False) + self.assertEqual(room_id_results[kick_room_id].newly_left, False) + + def test_state_reset(self) -> None: + """ + Test a state reset scenario where the user gets removed from the room (when + there is no corresponding leave event) + """ + 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") + + # The room where the state reset will happen + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + join_response1 = self.helper.join(room_id1, user1_id, tok=user1_tok) + + # Join another room so we don't hit the short-circuit and return early if they + # have no room membership + room_id2 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.join(room_id2, user1_id, tok=user1_tok) + + before_reset_token = self.event_sources.get_current_token() + + # Send another state event to make a position for the state reset to happen at + dummy_state_response = self.helper.send_state( + room_id1, + event_type="foobarbaz", + state_key="", + body={"foo": "bar"}, + tok=user2_tok, + ) + dummy_state_pos = self.get_success( + self.store.get_position_for_event(dummy_state_response["event_id"]) + ) + + # Mock a state reset removing the membership for user1 in the current state + self.get_success( + self.store.db_pool.simple_delete( + table="current_state_events", + keyvalues={ + "room_id": room_id1, + "type": EventTypes.Member, + "state_key": user1_id, + }, + desc="state reset user in current_state_events", + ) + ) + self.get_success( + self.store.db_pool.simple_delete( + table="local_current_membership", + keyvalues={ + "room_id": room_id1, + "user_id": user1_id, + }, + desc="state reset user in local_current_membership", + ) + ) + self.get_success( + self.store.db_pool.simple_insert( + table="current_state_delta_stream", + values={ + "stream_id": dummy_state_pos.stream, + "room_id": room_id1, + "type": EventTypes.Member, + "state_key": user1_id, + "event_id": None, + "prev_event_id": join_response1["event_id"], + "instance_name": dummy_state_pos.instance_name, + }, + desc="state reset user in current_state_delta_stream", + ) + ) + + # Manually bust the cache since we we're just manually messing with the database + # and not causing an actual state reset. + self.store._membership_stream_cache.entity_has_changed( + user1_id, dummy_state_pos.stream + ) + + after_reset_token = self.event_sources.get_current_token() + + # The function under test + room_id_results = self.get_success( + self.sliding_sync_handler.get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=before_reset_token, + to_token=after_reset_token, + ) + ) + + # Room1 should show up because it was `newly_left` via state reset during the from/to range + self.assertEqual(room_id_results.keys(), {room_id1, room_id2}) + # It should be pointing to no event because we were removed from the room + # without a corresponding leave event + self.assertEqual( + room_id_results[room_id1].event_id, + None, + ) + # State reset caused us to leave the room and there is no corresponding leave event + self.assertEqual(room_id_results[room_id1].membership, Membership.LEAVE) + # We should *NOT* be `newly_joined` because we joined before the token range + self.assertEqual(room_id_results[room_id1].newly_joined, False) + # We should be `newly_left` because we were removed via state reset during the from/to range + self.assertEqual(room_id_results[room_id1].newly_left, True) + + class FilterRoomsTestCase(HomeserverTestCase): """ Tests Sliding Sync handler `filter_rooms()` to make sure it includes/excludes rooms From 25f7d445ab9e939a90aefeb5dd83e26ea7aae04a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 10:44:06 -0500 Subject: [PATCH 25/39] Add changelog --- changelog.d/17432.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17432.feature diff --git a/changelog.d/17432.feature b/changelog.d/17432.feature new file mode 100644 index 0000000000..c86f04c118 --- /dev/null +++ b/changelog.d/17432.feature @@ -0,0 +1 @@ +Add room subscriptions to experimental [MSC3575](https://github.com/matrix-org/matrix-spec-proposals/pull/3575) Sliding Sync `/sync` endpoint. From a9122300a49d30e098833c6adff2404eba8ada5d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 11:00:27 -0500 Subject: [PATCH 26/39] Fix lints --- synapse/handlers/sliding_sync.py | 2 +- tests/rest/client/test_sync.py | 2 +- tests/unittest.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 5dd47ba403..2a3ef5fc59 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -684,7 +684,7 @@ async def get_room_membership_for_user_at_to_token( # - 1b) Add back rooms that the user left after the `to_token` # - 1c) Update room membership events to the point in time of the `to_token` # - 2) Figure out which rooms are `newly_left` rooms (> `from_token` and <= `to_token`) - # - 3) Figure out which rooms are `newly_joined` + # - 3) Figure out which rooms are `newly_joined` (> `from_token` and <= `to_token`) # - 4) Figure out which rooms are DM's # 1) Fetch membership changes that fall in the range from `to_token` up to diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index e2c2b90212..fbb0889300 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -20,7 +20,7 @@ # import json import logging -from typing import AbstractSet, Any, Dict, Iterable, List, Optional +from typing import Any, Dict, Iterable, List from parameterized import parameterized, parameterized_class diff --git a/tests/unittest.py b/tests/unittest.py index 2b28cf6ffa..4aa7f56106 100644 --- a/tests/unittest.py +++ b/tests/unittest.py @@ -28,8 +28,8 @@ import secrets import time from typing import ( - Any, AbstractSet, + Any, Awaitable, Callable, ClassVar, From 6036a11ccb682950cbe214a64916ec9338c2cf2a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 11:38:26 -0500 Subject: [PATCH 27/39] Split `get_room_membership_for_user_at_to_token(...)` from `filter_rooms_relevant_for_sync(...)` --- synapse/handlers/sliding_sync.py | 51 +++---- tests/handlers/test_sliding_sync.py | 219 +++++++++++++++++----------- 2 files changed, 162 insertions(+), 108 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 2a3ef5fc59..8b81004660 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -78,8 +78,9 @@ class _RoomMembershipForUser: membership: The membership state of the user in the room sender: The person who sent the membership event newly_joined: Whether the user newly joined the room during the given token - range + range and is still joined to the room at the end of this range. newly_left: Whether the user newly left the room during the given token range + and is still "leave" at the end of this range. is_dm: Whether this user considers this room as a direct-message (DM) room """ @@ -437,18 +438,26 @@ async def current_sync_for_user( # See https://github.com/matrix-org/matrix-doc/issues/1144 raise NotImplementedError() + # Get all of the room IDs that the user should be able to see in the sync + # response + if sync_config.lists is not None or sync_config.room_subscriptions is not None: + room_membership_for_user_map = ( + await self.get_room_membership_for_user_at_to_token( + user=sync_config.user, + to_token=to_token, + from_token=from_token, + ) + ) + # Assemble sliding window lists lists: Dict[str, SlidingSyncResult.SlidingWindowList] = {} # Keep track of the rooms that we're going to display and need to fetch more # info about relevant_room_map: Dict[str, RoomSyncConfig] = {} - if sync_config.lists: - # Get all of the room IDs that the user should be able to see in the sync - # response - sync_room_map = await self.get_sync_room_ids_for_user( - sync_config.user, - from_token=from_token, - to_token=to_token, + if sync_config.lists is not None: + sync_room_map = await self.filter_rooms_relevant_for_sync( + user=sync_config.user, + room_membership_for_user_map=room_membership_for_user_map, ) for list_key, list_config in sync_config.lists.items(): @@ -594,8 +603,8 @@ async def get_room_membership_for_user_at_to_token( from_token: Optional[StreamToken], ) -> Dict[str, _RoomMembershipForUser]: """ - Fetch room IDs that the user has had membership in (the full room list that will - be filtered, sorted, and sliced). + Fetch room IDs that the user has had membership in (the full room list including + long-lost left rooms that will be filtered, sorted, and sliced). We're looking for rooms where the user has had any sort of membership in the token range (> `from_token` and <= `to_token`) @@ -918,14 +927,13 @@ async def get_room_membership_for_user_at_to_token( return sync_room_id_set - async def get_sync_room_ids_for_user( + async def filter_rooms_relevant_for_sync( self, user: UserID, - to_token: StreamToken, - from_token: Optional[StreamToken], + room_membership_for_user_map: Dict[str, _RoomMembershipForUser], ) -> Dict[str, _RoomMembershipForUser]: """ - Fetch room IDs that should be listed for this user in the sync response (the + Filter room IDs that should/can be listed for this user in the sync response (the full room list that will be filtered, sorted, and sliced). We're looking for rooms where the user has the following state in the token @@ -943,8 +951,7 @@ async def get_sync_room_ids_for_user( Args: user: User to fetch rooms for - to_token: The token to fetch rooms up to. - from_token: The point in the stream to sync from. + room_membership_for_user_map: Returns: A dictionary of room IDs that should be listed in the sync response along @@ -952,23 +959,17 @@ async def get_sync_room_ids_for_user( """ user_id = user.to_string() - sync_room_id_set = await self.get_room_membership_for_user_at_to_token( - user=user, - to_token=to_token, - from_token=from_token, - ) - # Filter rooms to only what we're interested to sync with - filtered_sync_room_id_set = { + filtered_sync_room_map = { room_id: room_membership_for_user - for room_id, room_membership_for_user in sync_room_id_set.items() + for room_id, room_membership_for_user in room_membership_for_user_map.items() if filter_membership_for_sync( user_id=user_id, room_membership_for_user=room_membership_for_user, ) } - return filtered_sync_room_id_set + return filtered_sync_room_map async def check_room_subscription_allowed_for_user( self, user: UserID, room_id: str diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index e8f72cf7d3..3d8dc6a0f8 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -19,7 +19,7 @@ # import logging from copy import deepcopy -from typing import Optional +from typing import Dict, Optional from unittest.mock import patch from parameterized import parameterized @@ -35,12 +35,16 @@ RoomTypes, ) from synapse.api.room_versions import RoomVersions -from synapse.handlers.sliding_sync import RoomSyncConfig, StateValues +from synapse.handlers.sliding_sync import ( + _RoomMembershipForUser, + RoomSyncConfig, + StateValues, +) from synapse.rest import admin from synapse.rest.client import knock, login, room from synapse.server import HomeServer from synapse.storage.util.id_generators import MultiWriterIdGenerator -from synapse.types import JsonDict, UserID +from synapse.types import JsonDict, UserID, StreamToken from synapse.types.handlers import SlidingSyncConfig from synapse.util import Clock @@ -2628,9 +2632,9 @@ def test_sharded_event_persisters(self) -> None: self.assertEqual(room_id_results[room_id3].newly_left, False) -class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): +class FilterRoomsRelevantForSyncTestCase(HomeserverTestCase): """ - Tests Sliding Sync handler `get_sync_room_ids_for_user()` to make sure it returns + Tests Sliding Sync handler `filter_rooms_relevant_for_sync()` to make sure it returns the correct list of rooms IDs. """ @@ -2653,6 +2657,31 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.event_sources = hs.get_event_sources() self.storage_controllers = hs.get_storage_controllers() + def _get_sync_room_ids_for_user( + self, + user: UserID, + to_token: StreamToken, + from_token: Optional[StreamToken], + ) -> Dict[str, _RoomMembershipForUser]: + """ + Get the rooms the user should be syncing with + """ + room_membership_for_user_map = self.get_success( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( + user=user, + from_token=from_token, + to_token=to_token, + ) + ) + filtered_sync_room_map = self.get_success( + self.sliding_sync_handler.filter_rooms_relevant_for_sync( + user=user, + room_membership_for_user_map=room_membership_for_user_map, + ) + ) + + return filtered_sync_room_map + def test_no_rooms(self) -> None: """ Test when the user has never joined any rooms before @@ -2662,12 +2691,10 @@ def test_no_rooms(self) -> None: now_token = self.event_sources.get_current_token() - room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=now_token, - to_token=now_token, - ) + room_id_results = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=now_token, + to_token=now_token, ) self.assertEqual(room_id_results.keys(), set()) @@ -2729,12 +2756,10 @@ def test_basic_rooms(self) -> None: after_room_token = self.event_sources.get_current_token() - room_id_results = self.get_success( - self.sliding_sync_handler.get_room_membership_for_user_at_to_token( - UserID.from_string(user1_id), - from_token=before_room_token, - to_token=after_room_token, - ) + room_id_results = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=before_room_token, + to_token=after_room_token, ) # Ensure that the invited, ban, and knock rooms show up @@ -2802,12 +2827,10 @@ def test_only_newly_left_rooms_show_up(self) -> None: after_room2_token = self.event_sources.get_current_token() - room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=after_room1_token, - to_token=after_room2_token, - ) + room_id_results = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=after_room1_token, + to_token=after_room2_token, ) # Only the `newly_left` room should show up @@ -2849,12 +2872,10 @@ def test_get_kicked_room(self) -> None: after_kick_token = self.event_sources.get_current_token() - room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=after_kick_token, - to_token=after_kick_token, - ) + room_id_results = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=after_kick_token, + to_token=after_kick_token, ) # The kicked room should show up @@ -2951,12 +2972,10 @@ def test_state_reset(self) -> None: after_reset_token = self.event_sources.get_current_token() # The function under test - room_id_results = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=before_reset_token, - to_token=after_reset_token, - ) + room_id_results = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=before_reset_token, + to_token=after_reset_token, ) # Room1 should show up because it was `newly_left` via state reset during the from/to range @@ -2999,6 +3018,31 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = self.hs.get_datastores().main self.event_sources = hs.get_event_sources() + def _get_sync_room_ids_for_user( + self, + user: UserID, + to_token: StreamToken, + from_token: Optional[StreamToken], + ) -> Dict[str, _RoomMembershipForUser]: + """ + Get the rooms the user should be syncing with + """ + room_membership_for_user_map = self.get_success( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( + user=user, + from_token=from_token, + to_token=to_token, + ) + ) + filtered_sync_room_map = self.get_success( + self.sliding_sync_handler.filter_rooms_relevant_for_sync( + user=user, + room_membership_for_user_map=room_membership_for_user_map, + ) + ) + + return filtered_sync_room_map + def _create_dm_room( self, inviter_user_id: str, @@ -3070,12 +3114,10 @@ def test_filter_dm_rooms(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, - ) + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, ) # Try with `is_dm=True` @@ -3128,12 +3170,10 @@ def test_filter_encrypted_rooms(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, - ) + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, ) # Try with `is_encrypted=True` @@ -3184,12 +3224,10 @@ def test_filter_invite_rooms(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, - ) + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, ) # Try with `is_invite=True` @@ -3253,12 +3291,10 @@ def test_filter_room_types(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, - ) + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, ) # Try finding only normal rooms @@ -3346,12 +3382,10 @@ def test_filter_not_room_types(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, - ) + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, ) # Try finding *NOT* normal rooms @@ -3450,6 +3484,31 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.store = self.hs.get_datastores().main self.event_sources = hs.get_event_sources() + def _get_sync_room_ids_for_user( + self, + user: UserID, + to_token: StreamToken, + from_token: Optional[StreamToken], + ) -> Dict[str, _RoomMembershipForUser]: + """ + Get the rooms the user should be syncing with + """ + room_membership_for_user_map = self.get_success( + self.sliding_sync_handler.get_room_membership_for_user_at_to_token( + user=user, + from_token=from_token, + to_token=to_token, + ) + ) + filtered_sync_room_map = self.get_success( + self.sliding_sync_handler.filter_rooms_relevant_for_sync( + user=user, + room_membership_for_user_map=room_membership_for_user_map, + ) + ) + + return filtered_sync_room_map + def test_sort_activity_basic(self) -> None: """ Rooms with newer activity are sorted first. @@ -3469,12 +3528,10 @@ def test_sort_activity_basic(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, - ) + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, ) # Sort the rooms (what we're testing) @@ -3552,12 +3609,10 @@ def test_activity_after_xxx(self, room1_membership: str) -> None: self.helper.send(room_id3, "activity in room3", tok=user2_tok) # Get the rooms the user should be syncing with - sync_room_map = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=before_rooms_token, - to_token=after_rooms_token, - ) + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=before_rooms_token, + to_token=after_rooms_token, ) # Sort the rooms (what we're testing) @@ -3618,12 +3673,10 @@ def test_default_bump_event_types(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, - ) + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, ) # Sort the rooms (what we're testing) From 5fd19bbc6c420f8b5b8217a0a1f339ded20aedb5 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 12:02:40 -0500 Subject: [PATCH 28/39] Go back to iterating on room subscriptions --- synapse/handlers/sliding_sync.py | 146 +++++++++++++++------------- tests/handlers/test_sliding_sync.py | 4 +- 2 files changed, 79 insertions(+), 71 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 8b81004660..969e9a072d 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -440,7 +440,12 @@ async def current_sync_for_user( # Get all of the room IDs that the user should be able to see in the sync # response - if sync_config.lists is not None or sync_config.room_subscriptions is not None: + has_lists = sync_config.lists is not None and len(sync_config.lists) > 0 + has_room_subscriptions = ( + sync_config.room_subscriptions is not None + and len(sync_config.room_subscriptions) > 0 + ) + if has_lists or has_room_subscriptions: room_membership_for_user_map = ( await self.get_room_membership_for_user_at_to_token( user=sync_config.user, @@ -454,7 +459,7 @@ async def current_sync_for_user( # Keep track of the rooms that we're going to display and need to fetch more # info about relevant_room_map: Dict[str, RoomSyncConfig] = {} - if sync_config.lists is not None: + if has_lists and sync_config.lists is not None: sync_room_map = await self.filter_rooms_relevant_for_sync( user=sync_config.user, room_membership_for_user_map=room_membership_for_user_map, @@ -547,33 +552,39 @@ async def current_sync_for_user( ) # Handle room subscriptions - # if sync_config.room_subscriptions is not None: - # for room_id, room_subscription in sync_config.room_subscriptions.items(): - # # We can first check if they are already allowed to see the room based - # # on our previous work to assemble the `sync_room_map`. - # if sync_room_map.get(room_id) is None: - # # If not, we have to do some work to figure out if they should be - # # allowed to see the room. - # room_membership_for_user_at_to_token = ( - # await self.check_room_subscription_allowed_for_user( - # user=sync_config.user, room_id=room_id - # ) - # ) - - # # Skip this room if the user isn't allowed to see it - # if not room_membership_for_user_at_to_token: - # continue - - # # Take the superset of the `RoomSyncConfig` for each room. - # # - # # Update our `relevant_room_map` with the room we're going to display - # # and need to fetch more info about. - # room_sync_config = RoomSyncConfig.from_room_config(room_subscription) - # existing_room_sync_config = relevant_room_map.get(room_id) - # if existing_room_sync_config is not None: - # existing_room_sync_config.combine_room_sync_config(room_sync_config) - # else: - # relevant_room_map[room_id] = room_sync_config + if has_room_subscriptions and sync_config.room_subscriptions is not None: + for room_id, room_subscription in sync_config.room_subscriptions.items(): + # We can first check if they are already allowed to see the room based + # on our previous work to assemble the `sync_room_map`. + if sync_room_map.get(room_id) is None: + # If not, we have to do some work to figure out if they should be + # allowed to see the room. + room_membership_for_user_at_to_token = ( + await self.check_room_subscription_allowed_for_user( + room_id=room_id, + room_membership_for_user_map=room_membership_for_user_map, + to_token=to_token, + ) + ) + + # Skip this room if the user isn't allowed to see it + if not room_membership_for_user_at_to_token: + continue + + room_membership_for_user_map[room_id] = ( + room_membership_for_user_at_to_token + ) + + # Take the superset of the `RoomSyncConfig` for each room. + # + # Update our `relevant_room_map` with the room we're going to display + # and need to fetch more info about. + room_sync_config = RoomSyncConfig.from_room_config(room_subscription) + existing_room_sync_config = relevant_room_map.get(room_id) + if existing_room_sync_config is not None: + existing_room_sync_config.combine_room_sync_config(room_sync_config) + else: + relevant_room_map[room_id] = room_sync_config # Fetch room data rooms: Dict[str, SlidingSyncResult.RoomResult] = {} @@ -582,7 +593,9 @@ async def current_sync_for_user( user=sync_config.user, room_id=room_id, room_sync_config=room_sync_config, - room_membership_for_user_at_to_token=sync_room_map[room_id], + room_membership_for_user_at_to_token=room_membership_for_user_map[ + room_id + ], from_token=from_token, to_token=to_token, ) @@ -934,7 +947,7 @@ async def filter_rooms_relevant_for_sync( ) -> Dict[str, _RoomMembershipForUser]: """ Filter room IDs that should/can be listed for this user in the sync response (the - full room list that will be filtered, sorted, and sliced). + full room list that will be further filtered, sorted, and sliced). We're looking for rooms where the user has the following state in the token range (> `from_token` and <= `to_token`): @@ -951,7 +964,7 @@ async def filter_rooms_relevant_for_sync( Args: user: User to fetch rooms for - room_membership_for_user_map: + room_membership_for_user_map: TODO Returns: A dictionary of room IDs that should be listed in the sync response along @@ -972,7 +985,10 @@ async def filter_rooms_relevant_for_sync( return filtered_sync_room_map async def check_room_subscription_allowed_for_user( - self, user: UserID, room_id: str + self, + room_id: str, + room_membership_for_user_map: Dict[str, _RoomMembershipForUser], + to_token: StreamToken, ) -> Optional[_RoomMembershipForUser]: """ Check whether the user is allowed to see the room based on whether they have @@ -981,56 +997,48 @@ async def check_room_subscription_allowed_for_user( Similar to `check_user_in_room_or_world_readable(...)` Args: - TODO + room_id: Room to check + room_membership_for_user_map: TODO + to_token: The token to fetch rooms up to. Returns: TODO """ - user_id = user.to_string() # If they have had any membership in the room over time, let them # subscribe and see what they can. - ( - membership, - member_event_id, - ) = await self.store.get_local_current_membership_for_user_in_room( - user_id=user_id, - room_id=room_id, - ) - if membership is not None and member_event_id is not None: - return None - # return _RoomMembershipForUser( - # room_id=room_id, - # event_id=member_event_id, - # event_pos=room_for_user.event_pos, - # membership=membership, - # sender=room_for_user.sender, - # newly_joined=False, - # newly_left=False, - # is_dm=False, - # ) + existing_membership_for_user = room_membership_for_user_map.get(room_id) + if existing_membership_for_user is not None: + return existing_membership_for_user # If the room is `world_readable`, it doesn't matter whether they can join, # everyone can see the room. - visibility = await self.storage_controllers.state.get_current_state_event( - room_id, EventTypes.RoomHistoryVisibility, "" + not_in_room_membership_for_user = _RoomMembershipForUser( + room_id=room_id, + event_id=None, + event_pos=None, + membership=None, + sender=None, + newly_joined=False, + newly_left=False, + is_dm=False, + ) + room_state = await self.get_current_state_at( + room_id=room_id, + room_membership_for_user_at_to_token=not_in_room_membership_for_user, + state_filter=StateFilter.from_types( + [(EventTypes.RoomHistoryVisibility, "")] + ), + to_token=to_token, ) + + visibility_event = room_state.get((EventTypes.RoomHistoryVisibility, "")) if ( - visibility - and visibility.content.get("history_visibility") + visibility_event is not None + and visibility_event.content.get("history_visibility") == HistoryVisibility.WORLD_READABLE ): - return None - # return _RoomMembershipForUser( - # room_id=room_id, - # event_id=None, - # event_pos=None, - # membership=None, - # sender=None, - # newly_joined=False, - # newly_left=False, - # is_dm=False, - # ) + return not_in_room_membership_for_user return None diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 3d8dc6a0f8..59a410060b 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -36,15 +36,15 @@ ) from synapse.api.room_versions import RoomVersions from synapse.handlers.sliding_sync import ( - _RoomMembershipForUser, RoomSyncConfig, StateValues, + _RoomMembershipForUser, ) from synapse.rest import admin from synapse.rest.client import knock, login, room from synapse.server import HomeServer from synapse.storage.util.id_generators import MultiWriterIdGenerator -from synapse.types import JsonDict, UserID, StreamToken +from synapse.types import JsonDict, StreamToken, UserID from synapse.types.handlers import SlidingSyncConfig from synapse.util import Clock From 868dcdca07ada33d697f525ea08fea5bc8653f00 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 12:04:42 -0500 Subject: [PATCH 29/39] Revert `heroes` order changes --- synapse/storage/databases/main/roommember.py | 32 +++++--------------- 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/synapse/storage/databases/main/roommember.py b/synapse/storage/databases/main/roommember.py index 1d13de62f0..5d2fd08495 100644 --- a/synapse/storage/databases/main/roommember.py +++ b/synapse/storage/databases/main/roommember.py @@ -279,19 +279,8 @@ def _get_users_in_room_with_profiles( @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: @@ -319,28 +308,23 @@ def _get_room_summary_txn( for count, membership in txn: res.setdefault(membership, MemberSummary([], count)) - # Order by membership (joins -> invites -> leave (former insiders) -> everything else - # including knocks since they are outsiders), 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. + # 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. 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 WHEN ? THEN 3 ELSE 4 END ASC, - event_stream_ordering ASC + CASE membership WHEN ? THEN 1 WHEN ? THEN 2 ELSE 3 END ASC, + event_id 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, Membership.LEAVE, 6) - ) + txn.execute(sql, (room_id, Membership.JOIN, Membership.INVITE, 6)) for user_id, membership, event_id in txn: summary = res[membership] # we will always have a summary for this membership type at this From a4753bfb98dc5d727506605385532bd9b42c1715 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 12:06:22 -0500 Subject: [PATCH 30/39] Better comment on what we expect --- tests/rest/client/test_sync.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 674d966903..7afaef976f 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -2182,7 +2182,8 @@ def test_rooms_meta_heroes_max(self) -> None: hero["user_id"] for hero in channel.json_body["rooms"][room_id1].get("heroes", []) ], - # Heroes shouldn't include the user themselves (we shouldn't see user1) + # 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( From e2138b7dcf2f9b8dc4c43095e5d03ef640b1adce Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 12:20:17 -0500 Subject: [PATCH 31/39] Have to use basic assertion for now See https://github.com/element-hq/synapse/pull/17419#discussion_r1674387354 --- tests/rest/client/test_sync.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index 4ac11d716d..0d0bea538b 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -2186,15 +2186,18 @@ def test_rooms_meta_heroes_max(self) -> None: # Room2 doesn't have a name so we should see `heroes` populated self.assertIsNone(channel.json_body["rooms"][room_id1].get("name")) - 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], - ) + # 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.assertEqual( channel.json_body["rooms"][room_id1]["joined_count"], 7, From 2b7ec13a00795bdca6e5217eb9e50f27a9b7c2b1 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 14:15:04 -0500 Subject: [PATCH 32/39] Fix merge --- synapse/handlers/sliding_sync.py | 125 ++++++++++++++++-- tests/rest/client/test_sync.py | 220 +++++++++++++++++++++++++++++-- 2 files changed, 322 insertions(+), 23 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 51e6a2bd1c..904787ced3 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -24,13 +24,7 @@ import attr from immutabledict import immutabledict -from synapse.api.constants import ( - AccountDataTypes, - Direction, - EventContentFields, - EventTypes, - Membership, -) +from synapse.api.constants import AccountDataTypes, Direction, EventTypes, Membership from synapse.events import EventBase from synapse.events.utils import strip_event from synapse.handlers.relations import BundledAggregations @@ -546,11 +540,15 @@ async def current_sync_for_user( rooms[room_id] = room_sync_result + extensions = await self.get_extensions_response( + sync_config=sync_config, to_token=to_token + ) + return SlidingSyncResult( next_pos=to_token, lists=lists, rooms=rooms, - extensions={}, + extensions=extensions, ) async def get_sync_room_ids_for_user( @@ -975,11 +973,15 @@ async def filter_rooms( # provided in the list. `None` is a valid type for rooms which do not have a # room type. if filters.room_types is not None or filters.not_room_types is not None: - # Make a copy so we don't run into an error: `Set changed size during - # iteration`, when we filter out and remove items - for room_id in filtered_room_id_set.copy(): - create_event = await self.store.get_create_event_for_room(room_id) - room_type = create_event.content.get(EventContentFields.ROOM_TYPE) + room_to_type = await self.store.bulk_get_room_type( + { + room_id + for room_id in filtered_room_id_set + # We only know the room types for joined rooms + if sync_room_map[room_id].membership == Membership.JOIN + } + ) + for room_id, room_type in room_to_type.items(): if ( filters.room_types is not None and room_type not in filters.room_types @@ -1576,3 +1578,100 @@ async def get_room_sync_data( notification_count=0, highlight_count=0, ) + + async def get_extensions_response( + self, + sync_config: SlidingSyncConfig, + to_token: StreamToken, + ) -> SlidingSyncResult.Extensions: + """Handle extension requests. + + Args: + sync_config: Sync configuration + to_token: The point in the stream to sync up to. + """ + + if sync_config.extensions is None: + return SlidingSyncResult.Extensions() + + to_device_response = None + if sync_config.extensions.to_device: + to_device_response = await self.get_to_device_extensions_response( + sync_config=sync_config, + to_device_request=sync_config.extensions.to_device, + to_token=to_token, + ) + + return SlidingSyncResult.Extensions(to_device=to_device_response) + + async def get_to_device_extensions_response( + self, + sync_config: SlidingSyncConfig, + to_device_request: SlidingSyncConfig.Extensions.ToDeviceExtension, + to_token: StreamToken, + ) -> SlidingSyncResult.Extensions.ToDeviceExtension: + """Handle to-device extension (MSC3885) + + Args: + sync_config: Sync configuration + to_device_request: The to-device extension from the request + to_token: The point in the stream to sync up to. + """ + + user_id = sync_config.user.to_string() + device_id = sync_config.device_id + + # Check that this request has a valid device ID (not all requests have + # to belong to a device, and so device_id is None), and that the + # extension is enabled. + if device_id is None or not to_device_request.enabled: + return SlidingSyncResult.Extensions.ToDeviceExtension( + next_batch=f"{to_token.to_device_key}", + events=[], + ) + + since_stream_id = 0 + if to_device_request.since is not None: + # We've already validated this is an int. + since_stream_id = int(to_device_request.since) + + if to_token.to_device_key < since_stream_id: + # The since token is ahead of our current token, so we return an + # empty response. + logger.warning( + "Got to-device.since from the future. since token: %r is ahead of our current to_device stream position: %r", + since_stream_id, + to_token.to_device_key, + ) + return SlidingSyncResult.Extensions.ToDeviceExtension( + next_batch=to_device_request.since, + events=[], + ) + + # Delete everything before the given since token, as we know the + # device must have received them. + deleted = await self.store.delete_messages_for_device( + user_id=user_id, + device_id=device_id, + up_to_stream_id=since_stream_id, + ) + + logger.debug( + "Deleted %d to-device messages up to %d for %s", + deleted, + since_stream_id, + user_id, + ) + + messages, stream_id = await self.store.get_messages_for_device( + user_id=user_id, + device_id=device_id, + from_stream_id=since_stream_id, + to_stream_id=to_token.to_device_key, + limit=min(to_device_request.limit, 100), # Limit to at most 100 events + ) + + return SlidingSyncResult.Extensions.ToDeviceExtension( + next_batch=f"{stream_id}", + events=messages, + ) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index bec4858acd..4236812db5 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -38,7 +38,16 @@ ) from synapse.events import EventBase from synapse.handlers.sliding_sync import StateValues -from synapse.rest.client import devices, knock, login, read_marker, receipts, room, sync +from synapse.rest.client import ( + devices, + knock, + login, + read_marker, + receipts, + room, + sendtodevice, + sync, +) from synapse.server import HomeServer from synapse.types import JsonDict, RoomStreamToken, StreamKeyType, StreamToken, UserID from synapse.util import Clock @@ -47,7 +56,7 @@ from tests.federation.transport.test_knocking import ( KnockingStrippedStateEventHelperMixin, ) -from tests.server import TimedOutException +from tests.server import FakeChannel, TimedOutException from tests.test_utils.event_injection import mark_event_as_partial_state logger = logging.getLogger(__name__) @@ -2200,14 +2209,18 @@ def test_rooms_meta_heroes_max(self) -> None: # Room2 doesn't have a name so we should see `heroes` populated self.assertIsNone(channel.json_body["rooms"][room_id1].get("name")) - self.assertCountEqual( - [ - hero["user_id"] - for hero in channel.json_body["rooms"][room_id1].get("heroes", []) - ], - # Heroes shouldn't include the user themselves (we shouldn't see user1) - [user2_id, user3_id, user4_id, user5_id, user6_id], - ) + # 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.assertEqual( channel.json_body["rooms"][room_id1]["joined_count"], 7, @@ -3995,3 +4008,190 @@ def test_rooms_required_state_partial_state(self) -> None: ], channel.json_body["lists"]["foo-list"], ) + + +class SlidingSyncToDeviceExtensionTestCase(unittest.HomeserverTestCase): + """Tests for the to-device sliding sync extension""" + + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + sync.register_servlets, + sendtodevice.register_servlets, + ] + + def default_config(self) -> JsonDict: + config = super().default_config() + # Enable sliding sync + config["experimental_features"] = {"msc3575_enabled": True} + return config + + def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: + self.store = hs.get_datastores().main + self.sync_endpoint = ( + "/_matrix/client/unstable/org.matrix.simplified_msc3575/sync" + ) + + def _assert_to_device_response( + self, channel: FakeChannel, expected_messages: List[JsonDict] + ) -> str: + """Assert the sliding sync response was successful and has the expected + to-device messages. + + Returns the next_batch token from the to-device section. + """ + self.assertEqual(channel.code, 200, channel.json_body) + extensions = channel.json_body["extensions"] + to_device = extensions["to_device"] + self.assertIsInstance(to_device["next_batch"], str) + self.assertEqual(to_device["events"], expected_messages) + + return to_device["next_batch"] + + def test_no_data(self) -> None: + """Test that enabling to-device extension works, even if there is + no-data + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "to_device": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + + # We expect no to-device messages + self._assert_to_device_response(channel, []) + + def test_data_initial_sync(self) -> None: + """Test that we get to-device messages when we don't specify a since + token""" + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass", "d1") + user2_id = self.register_user("u2", "pass") + user2_tok = self.login(user2_id, "pass", "d2") + + # Send the to-device message + test_msg = {"foo": "bar"} + chan = self.make_request( + "PUT", + "/_matrix/client/r0/sendToDevice/m.test/1234", + content={"messages": {user1_id: {"d1": test_msg}}}, + access_token=user2_tok, + ) + self.assertEqual(chan.code, 200, chan.result) + + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "to_device": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + self._assert_to_device_response( + channel, + [{"content": test_msg, "sender": user2_id, "type": "m.test"}], + ) + + def test_data_incremental_sync(self) -> None: + """Test that we get to-device messages over incremental syncs""" + + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass", "d1") + user2_id = self.register_user("u2", "pass") + user2_tok = self.login(user2_id, "pass", "d2") + + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "to_device": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + # No to-device messages yet. + next_batch = self._assert_to_device_response(channel, []) + + test_msg = {"foo": "bar"} + chan = self.make_request( + "PUT", + "/_matrix/client/r0/sendToDevice/m.test/1234", + content={"messages": {user1_id: {"d1": test_msg}}}, + access_token=user2_tok, + ) + self.assertEqual(chan.code, 200, chan.result) + + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "to_device": { + "enabled": True, + "since": next_batch, + } + }, + }, + access_token=user1_tok, + ) + next_batch = self._assert_to_device_response( + channel, + [{"content": test_msg, "sender": user2_id, "type": "m.test"}], + ) + + # The next sliding sync request should not include the to-device + # message. + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "to_device": { + "enabled": True, + "since": next_batch, + } + }, + }, + access_token=user1_tok, + ) + self._assert_to_device_response(channel, []) + + # An initial sliding sync request should not include the to-device + # message, as it should have been deleted + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "lists": {}, + "extensions": { + "to_device": { + "enabled": True, + } + }, + }, + access_token=user1_tok, + ) + self._assert_to_device_response(channel, []) From 53a04fed55c7b03c980759efdec1fe57e0b7e382 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 14:34:58 -0500 Subject: [PATCH 33/39] Fix tests --- synapse/handlers/sliding_sync.py | 38 ++++++++++++++--------------- tests/handlers/test_sliding_sync.py | 10 +++----- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index b972c30f4b..fdf33fa23c 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -553,26 +553,23 @@ async def current_sync_for_user( # Handle room subscriptions if has_room_subscriptions and sync_config.room_subscriptions is not None: for room_id, room_subscription in sync_config.room_subscriptions.items(): - # We can first check if they are already allowed to see the room based - # on our previous work to assemble the `sync_room_map`. - if sync_room_map.get(room_id) is None: - # If not, we have to do some work to figure out if they should be - # allowed to see the room. - room_membership_for_user_at_to_token = ( - await self.check_room_subscription_allowed_for_user( - room_id=room_id, - room_membership_for_user_map=room_membership_for_user_map, - to_token=to_token, - ) + # If not, we have to do some work to figure out if they should be + # allowed to see the room. + room_membership_for_user_at_to_token = ( + await self.check_room_subscription_allowed_for_user( + room_id=room_id, + room_membership_for_user_map=room_membership_for_user_map, + to_token=to_token, ) + ) - # Skip this room if the user isn't allowed to see it - if not room_membership_for_user_at_to_token: - continue + # Skip this room if the user isn't allowed to see it + if not room_membership_for_user_at_to_token: + continue - room_membership_for_user_map[room_id] = ( - room_membership_for_user_at_to_token - ) + room_membership_for_user_map[room_id] = ( + room_membership_for_user_at_to_token + ) # Take the superset of the `RoomSyncConfig` for each room. # @@ -1008,8 +1005,11 @@ async def check_room_subscription_allowed_for_user( TODO """ - # If they have had any membership in the room over time, let them - # subscribe and see what they can. + # We can first check if they are already allowed to see the room based + # on our previous work to assemble the `room_membership_for_user_map`. + # + # If they have had any membership in the room over time (up to the `to_token`), + # let them subscribe and see what they can. existing_membership_for_user = room_membership_for_user_map.get(room_id) if existing_membership_for_user is not None: return existing_membership_for_user diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 1e43dc6e9e..a7aa9bb8af 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -3506,12 +3506,10 @@ def test_filter_room_types_with_invite_remote_room(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self.get_success( - self.sliding_sync_handler.get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, - ) + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, ) filtered_room_map = self.get_success( From 7495430b18b098c35eea27369f85b7c027dba25e Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 14:56:19 -0500 Subject: [PATCH 34/39] Explain why optional --- synapse/handlers/sliding_sync.py | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index fdf33fa23c..4112844e18 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -72,6 +72,7 @@ class _RoomMembershipForUser: """ Attributes: + room_id: The room ID of the membership event event_id: The event ID of the membership event event_pos: The stream position of the membership event membership: The membership state of the user in the room @@ -84,9 +85,21 @@ class _RoomMembershipForUser: """ room_id: str + # Optional because you can view `world_readable` rooms without joining them + # AND because state resets can affect room membership without a corresponding event. event_id: Optional[str] - event_pos: PersistedEventPosition - membership: str + # Optional because you can view `world_readable` rooms without joining them. Even + # during a state reset which removes the user from the room, we expect this to be + # set because `current_state_delta_stream` will note the position that the reset + # happened. + event_pos: Optional[PersistedEventPosition] + # Optional because you can view `world_readable` rooms without joining them. Even + # during a state reset which removes the user from the room, we expect this to be + # set to `LEAVE` because we can make that assumption based on the situaton (see + # `get_current_state_delta_membership_changes_for_user(...)`) + membership: Optional[str] + # Optional because you can view `world_readable` rooms without joining them + # AND because state resets can affect room membership without a corresponding event. sender: Optional[str] newly_joined: bool newly_left: bool @@ -963,8 +976,8 @@ async def filter_rooms_relevant_for_sync( from/to range. Args: - user: User to fetch rooms for - room_membership_for_user_map: TODO + user: User that is syncing + room_membership_for_user_map: Room membership for the user Returns: A dictionary of room IDs that should be listed in the sync response along @@ -998,11 +1011,13 @@ async def check_room_subscription_allowed_for_user( Args: room_id: Room to check - room_membership_for_user_map: TODO + room_membership_for_user_map: Room membership for the user at the time of + the `to_token` (<= `to_token`). to_token: The token to fetch rooms up to. Returns: - TODO + The room membership for the user if they are allowed to subscribe to the + room else `None`. """ # We can first check if they are already allowed to see the room based From d136f73865027edc74fcf256aebc79f1edaf2dce Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 16:02:21 -0500 Subject: [PATCH 35/39] Update comments --- synapse/handlers/sliding_sync.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 4112844e18..72d3f7a2e9 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -79,8 +79,8 @@ class _RoomMembershipForUser: sender: The person who sent the membership event newly_joined: Whether the user newly joined the room during the given token range and is still joined to the room at the end of this range. - newly_left: Whether the user newly left the room during the given token range - and is still "leave" at the end of this range. + newly_left: Whether the user newly left (or kicked) the room during the given + token range and is still "leave" at the end of this range. is_dm: Whether this user considers this room as a direct-message (DM) room """ @@ -131,14 +131,20 @@ def filter_membership_for_sync( # leave event. # # A leave != kick. This logic includes kicks (leave events where the sender is not - # the same user) and can be read as "anything that isn't a leave or newly_left or a - # leave with a different sender". + # the same user). # # When `sender=None`, it means that a state reset happened that removed the user # from the room without a corresponding leave event. We can just remove the rooms # since they are no longer relevant to the user but will still appear if they are # `newly_left`. - return membership != Membership.LEAVE or newly_left or sender not in (user_id, None) + return ( + # Anything except leave events + membership != Membership.LEAVE + # Unless... + or newly_left + # Allow kicks + or (membership == Membership.LEAVE and sender not in (user_id, None)) + ) # We can't freeze this class because we want to update it in place with the @@ -1258,7 +1264,6 @@ async def get_current_state_ids_at( in the room at the time of `to_token`. to_token: The point in the stream to sync up to. """ - room_state_ids: StateMap[str] # People shouldn't see past their leave/ban event if room_membership_for_user_at_to_token.membership in ( From dc5b8ad29367e63b8b1bb4f78c3da5faa333bd5d Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 16:05:24 -0500 Subject: [PATCH 36/39] Handle `world_readable` room subscriptions in the future --- synapse/handlers/sliding_sync.py | 82 +++++++++++++++----------------- 1 file changed, 39 insertions(+), 43 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 72d3f7a2e9..00ec082609 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -28,7 +28,6 @@ AccountDataTypes, Direction, EventTypes, - HistoryVisibility, Membership, ) from synapse.events import EventBase @@ -85,21 +84,17 @@ class _RoomMembershipForUser: """ room_id: str - # Optional because you can view `world_readable` rooms without joining them - # AND because state resets can affect room membership without a corresponding event. + # Optional because state resets can affect room membership without a corresponding event. event_id: Optional[str] - # Optional because you can view `world_readable` rooms without joining them. Even - # during a state reset which removes the user from the room, we expect this to be - # set because `current_state_delta_stream` will note the position that the reset + # Even during a state reset which removes the user from the room, we expect this to + # be set because `current_state_delta_stream` will note the position that the reset # happened. - event_pos: Optional[PersistedEventPosition] - # Optional because you can view `world_readable` rooms without joining them. Even - # during a state reset which removes the user from the room, we expect this to be - # set to `LEAVE` because we can make that assumption based on the situaton (see + event_pos: PersistedEventPosition + # Even during a state reset which removes the user from the room, we expect this to + # be set to `LEAVE` because we can make that assumption based on the situaton (see # `get_current_state_delta_membership_changes_for_user(...)`) - membership: Optional[str] - # Optional because you can view `world_readable` rooms without joining them - # AND because state resets can affect room membership without a corresponding event. + membership: str + # Optional because state resets can affect room membership without a corresponding event. sender: Optional[str] newly_joined: bool newly_left: bool @@ -572,8 +567,6 @@ async def current_sync_for_user( # Handle room subscriptions if has_room_subscriptions and sync_config.room_subscriptions is not None: for room_id, room_subscription in sync_config.room_subscriptions.items(): - # If not, we have to do some work to figure out if they should be - # allowed to see the room. room_membership_for_user_at_to_token = ( await self.check_room_subscription_allowed_for_user( room_id=room_id, @@ -1035,36 +1028,39 @@ async def check_room_subscription_allowed_for_user( if existing_membership_for_user is not None: return existing_membership_for_user + # TODO: Handle `world_readable` rooms + return None + # If the room is `world_readable`, it doesn't matter whether they can join, # everyone can see the room. - not_in_room_membership_for_user = _RoomMembershipForUser( - room_id=room_id, - event_id=None, - event_pos=None, - membership=None, - sender=None, - newly_joined=False, - newly_left=False, - is_dm=False, - ) - room_state = await self.get_current_state_at( - room_id=room_id, - room_membership_for_user_at_to_token=not_in_room_membership_for_user, - state_filter=StateFilter.from_types( - [(EventTypes.RoomHistoryVisibility, "")] - ), - to_token=to_token, - ) - - visibility_event = room_state.get((EventTypes.RoomHistoryVisibility, "")) - if ( - visibility_event is not None - and visibility_event.content.get("history_visibility") - == HistoryVisibility.WORLD_READABLE - ): - return not_in_room_membership_for_user - - return None + # not_in_room_membership_for_user = _RoomMembershipForUser( + # room_id=room_id, + # event_id=None, + # event_pos=None, + # membership=None, + # sender=None, + # newly_joined=False, + # newly_left=False, + # is_dm=False, + # ) + # room_state = await self.get_current_state_at( + # room_id=room_id, + # room_membership_for_user_at_to_token=not_in_room_membership_for_user, + # state_filter=StateFilter.from_types( + # [(EventTypes.RoomHistoryVisibility, "")] + # ), + # to_token=to_token, + # ) + + # visibility_event = room_state.get((EventTypes.RoomHistoryVisibility, "")) + # if ( + # visibility_event is not None + # and visibility_event.content.get("history_visibility") + # == HistoryVisibility.WORLD_READABLE + # ): + # return not_in_room_membership_for_user + + # return None async def filter_rooms( self, From d433917262071dfe17bbb2cbda8c9f18f2b051c3 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 16:09:32 -0500 Subject: [PATCH 37/39] Fix lints --- synapse/handlers/sliding_sync.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 00ec082609..be98b379eb 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -24,12 +24,7 @@ import attr from immutabledict import immutabledict -from synapse.api.constants import ( - AccountDataTypes, - Direction, - EventTypes, - Membership, -) +from synapse.api.constants import AccountDataTypes, Direction, EventTypes, Membership from synapse.events import EventBase from synapse.events.utils import strip_event from synapse.handlers.relations import BundledAggregations From 0bc4e36414bd26f817c0083365fbdb6748c10d39 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 18:18:15 -0500 Subject: [PATCH 38/39] Add tests --- tests/rest/client/test_sync.py | 292 +++++++++++++++++++++++++++++++-- 1 file changed, 282 insertions(+), 10 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index aa4f28f58e..f5175c32e2 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -22,6 +22,8 @@ import logging from typing import Any, Dict, Iterable, List +from http import HTTPStatus + from parameterized import parameterized, parameterized_class from twisted.test.proto_helpers import MemoryReactor @@ -3818,6 +3820,13 @@ def test_rooms_required_state_combine_superset(self) -> None: body={"foo": "bar"}, tok=user2_tok, ) + self.helper.send_state( + room_id1, + event_type="org.matrix.bar_state", + state_key="", + body={"bar": "qux"}, + tok=user2_tok, + ) # Make the Sliding Sync request with wildcards for the `state_key` channel = self.make_request( @@ -3841,16 +3850,13 @@ def test_rooms_required_state_combine_superset(self) -> None: ], "timeline_limit": 0, }, - } - # TODO: Room subscription should also combine with the `required_state` - # "room_subscriptions": { - # room_id1: { - # "required_state": [ - # ["org.matrix.bar_state", ""] - # ], - # "timeline_limit": 0, - # } - # } + }, + "room_subscriptions": { + room_id1: { + "required_state": [["org.matrix.bar_state", ""]], + "timeline_limit": 0, + } + }, }, access_token=user1_tok, ) @@ -3867,6 +3873,7 @@ def test_rooms_required_state_combine_superset(self) -> None: state_map[(EventTypes.Member, user1_id)], state_map[(EventTypes.Member, user2_id)], state_map[("org.matrix.foo_state", "")], + state_map[("org.matrix.bar_state", "")], }, exact=True, ) @@ -3959,6 +3966,271 @@ def test_rooms_required_state_partial_state(self) -> None: channel.json_body["lists"]["foo-list"], ) + def test_room_subscriptions_with_join_membership(self) -> None: + """ + Test `room_subscriptions` with a joined room should give us timeline and current + state events. + """ + 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") + + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + join_response = self.helper.join(room_id1, user1_id, tok=user1_tok) + + # Make the Sliding Sync request with just the room subscription + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "room_subscriptions": { + room_id1: { + "required_state": [ + [EventTypes.Create, ""], + ], + "timeline_limit": 1, + } + }, + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + # We should see some state + self._assertRequiredStateIncludes( + channel.json_body["rooms"][room_id1]["required_state"], + { + state_map[(EventTypes.Create, "")], + }, + exact=True, + ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) + + # We should see some events + self.assertEqual( + [ + event["event_id"] + for event in channel.json_body["rooms"][room_id1]["timeline"] + ], + [ + join_response["event_id"], + ], + channel.json_body["rooms"][room_id1]["timeline"], + ) + # No "live" events in an initial sync (no `from_token` to define the "live" + # range) + self.assertEqual( + channel.json_body["rooms"][room_id1]["num_live"], + 0, + channel.json_body["rooms"][room_id1], + ) + # There are more events to paginate to + self.assertEqual( + channel.json_body["rooms"][room_id1]["limited"], + True, + channel.json_body["rooms"][room_id1], + ) + + def test_room_subscriptions_with_leave_membership(self) -> None: + """ + Test `room_subscriptions` with a leave room should give us timeline and state + events up to the leave event. + """ + 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") + + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + self.helper.send_state( + room_id1, + event_type="org.matrix.foo_state", + state_key="", + body={"foo": "bar"}, + tok=user2_tok, + ) + + join_response = self.helper.join(room_id1, user1_id, tok=user1_tok) + leave_response = self.helper.leave(room_id1, user1_id, tok=user1_tok) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + # Send some events after user1 leaves + self.helper.send(room_id1, "activity after leave", tok=user2_tok) + # Update state after user1 leaves + self.helper.send_state( + room_id1, + event_type="org.matrix.foo_state", + state_key="", + body={"foo": "qux"}, + tok=user2_tok, + ) + + # Make the Sliding Sync request with just the room subscription + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "room_subscriptions": { + room_id1: { + "required_state": [ + ["org.matrix.foo_state", ""], + ], + "timeline_limit": 2, + } + }, + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # We should see the state at the time of the leave + self._assertRequiredStateIncludes( + channel.json_body["rooms"][room_id1]["required_state"], + { + state_map[("org.matrix.foo_state", "")], + }, + exact=True, + ) + self.assertIsNone(channel.json_body["rooms"][room_id1].get("invite_state")) + + # We should see some before we left (nothing after) + self.assertEqual( + [ + event["event_id"] + for event in channel.json_body["rooms"][room_id1]["timeline"] + ], + [ + join_response["event_id"], + leave_response["event_id"], + ], + channel.json_body["rooms"][room_id1]["timeline"], + ) + # No "live" events in an initial sync (no `from_token` to define the "live" + # range) + self.assertEqual( + channel.json_body["rooms"][room_id1]["num_live"], + 0, + channel.json_body["rooms"][room_id1], + ) + # There are more events to paginate to + self.assertEqual( + channel.json_body["rooms"][room_id1]["limited"], + True, + channel.json_body["rooms"][room_id1], + ) + + def test_room_subscriptions_no_leak_private_room(self) -> None: + """ + Test `room_subscriptions` with a private room we have never been in should not + leak any data to the user. + """ + 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") + + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=False) + + # We should not be able to join the private room + self.helper.join( + room_id1, user1_id, tok=user1_tok, expect_code=HTTPStatus.FORBIDDEN + ) + + # Make the Sliding Sync request with just the room subscription + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "room_subscriptions": { + room_id1: { + "required_state": [ + [EventTypes.Create, ""], + ], + "timeline_limit": 1, + } + }, + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # We should not see the room at all (we're not in it) + self.assertIsNone( + channel.json_body["rooms"].get(room_id1), channel.json_body["rooms"] + ) + + def test_room_subscriptions_world_readable(self) -> None: + """ + Test `room_subscriptions` with a room that has `world_readable` history visibility + + FIXME: We should be able to see the room timeline and state + """ + 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") + + # Create a room with `world_readable` history visibility + room_id1 = self.helper.create_room_as( + user2_id, + tok=user2_tok, + extra_content={ + "preset": "public_chat", + "initial_state": [ + { + "content": { + "history_visibility": HistoryVisibility.WORLD_READABLE + }, + "state_key": "", + "type": EventTypes.RoomHistoryVisibility, + } + ], + }, + ) + # Ensure we're testing with a room with `world_readable` history visibility + # which means events are visible to anyone even without membership. + history_visibility_response = self.helper.get_state( + room_id1, EventTypes.RoomHistoryVisibility, tok=user2_tok + ) + self.assertEqual( + history_visibility_response.get("history_visibility"), + HistoryVisibility.WORLD_READABLE, + ) + + # Note: We never join the room + + # Make the Sliding Sync request with just the room subscription + channel = self.make_request( + "POST", + self.sync_endpoint, + { + "room_subscriptions": { + room_id1: { + "required_state": [ + [EventTypes.Create, ""], + ], + "timeline_limit": 1, + } + }, + }, + access_token=user1_tok, + ) + self.assertEqual(channel.code, 200, channel.json_body) + + # FIXME: In the future, we should be able to see the room because it's + # `world_readable` but currently we don't support this. + self.assertIsNone( + channel.json_body["rooms"].get(room_id1), channel.json_body["rooms"] + ) + class SlidingSyncToDeviceExtensionTestCase(unittest.HomeserverTestCase): """Tests for the to-device sliding sync extension""" From 3fa59dd7033586c3e6ec120a114b720f44a67cc4 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 11 Jul 2024 18:18:50 -0500 Subject: [PATCH 39/39] Fix lints --- tests/rest/client/test_sync.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/rest/client/test_sync.py b/tests/rest/client/test_sync.py index f5175c32e2..f5d57e689c 100644 --- a/tests/rest/client/test_sync.py +++ b/tests/rest/client/test_sync.py @@ -20,9 +20,8 @@ # import json import logging -from typing import Any, Dict, Iterable, List - from http import HTTPStatus +from typing import Any, Dict, Iterable, List from parameterized import parameterized, parameterized_class