From e82607f31c3ff6cd434185d6906d809cfb335440 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 19 Nov 2024 16:20:40 -0600 Subject: [PATCH 1/7] Add test to see what happens when membership change is included in the timeline Part of https://github.com/element-hq/synapse/issues/17929 --- synapse/api/constants.py | 2 + synapse/handlers/sliding_sync/__init__.py | 11 +++ .../sliding_sync/test_rooms_required_state.py | 90 ++++++++++++++++++- 3 files changed, 101 insertions(+), 2 deletions(-) diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 8db302b3d8b..1206d1e00f3 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -231,6 +231,8 @@ class EventContentFields: ROOM_NAME: Final = "name" MEMBERSHIP: Final = "membership" + MEMBERSHIP_DISPLAYNAME: Final = "displayname" + MEMBERSHIP_AVATAR_URL: Final = "avatar_url" # Used in m.room.guest_access events. GUEST_ACCESS: Final = "guest_access" diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 85cfbc6dbf5..46a41d6623c 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -955,6 +955,17 @@ async def get_room_sync_data( and state_key == StateValues.LAZY ): lazy_load_room_members = True + + # For incremental syncs that aren't limited, when + # lazy-loading room members, also include any membership + # that has changed. This allows clients to cache the + # membership list for as long as it doesn't get a gappy + # sync, but still ensures for large gaps the server doesn't + # need to send down all membership changes. + # if not initial and not limited: + # # `None` is a wildcard in the `StateFilter` + # required_state_types.append((EventTypes.Member, None)) + # Everyone in the timeline is relevant # # FIXME: We probably also care about invite, ban, kick, targets, etc diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index ecea5f2d5b3..a7f06e15619 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -18,7 +18,7 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import EventTypes, Membership +from synapse.api.constants import EventTypes, Membership, EventContentFields from synapse.handlers.sliding_sync import StateValues from synapse.rest.client import login, room, sync from synapse.server import HomeServer @@ -496,6 +496,92 @@ def test_rooms_required_state_lazy_loading_room_members_incremental_sync( ) self.assertIsNone(response_body["rooms"][room_id1].get("invite_state")) + def test_rooms_required_state_changed_membership_in_timeline_lazy_loading_room_members_incremental_sync( + self, + ) -> None: + """ + On incremental sync, test `rooms.required_state` returns people relevant to the + timeline when lazy-loading room members, `["m.room.member","$LAZY"]` **including + changes to membership**. + """ + 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") + + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + 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.send(room_id1, "1", tok=user2_tok) + self.helper.send(room_id1, "2", tok=user2_tok) + self.helper.send(room_id1, "3", tok=user2_tok) + + # Make the Sliding Sync request with lazy loading for the room members + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 1]], + "required_state": [ + [EventTypes.Create, ""], + [EventTypes.Member, StateValues.LAZY], + ], + "timeline_limit": 3, + } + } + } + response_body, from_token = self.do_sync(sync_body, tok=user1_tok) + + # Send more timeline events into the room + self.helper.send(room_id1, "4", tok=user2_tok) + self.helper.send(room_id1, "5", tok=user4_tok) + # self.helper.send(room_id1, "6", tok=user4_tok) + # Update the display name of user5 (causing a membership change) + self.helper.send_state( + room_id1, + event_type=EventTypes.Member, + state_key=user5_id, + body={ + EventContentFields.MEMBERSHIP: "join", + EventContentFields.MEMBERSHIP_DISPLAYNAME: "quick changer", + }, + tok=user5_tok, + ) + + # Make an incremental Sliding Sync request + response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) + + state_map = self.get_success( + self.storage_controllers.state.get_current_state(room_id1) + ) + + # Only user2, user4, and user5 sent events in the last 3 events we see in the + # `timeline`. + self._assertRequiredStateIncludes( + response_body["rooms"][room_id1]["required_state"], + { + # This appears because *some* membership in the room changed and the + # heroes are recalculated and is thrown in because we have it. But this + # is technically optional and not needed because we've already seen user2 + # in the last sync (and their membership hasn't changed). + state_map[(EventTypes.Member, user2_id)], + # Appears because there is a message in the timeline from this user + state_map[(EventTypes.Member, user4_id)], + # Appears because there is a membership event in the timeline from this user + state_map[(EventTypes.Member, user5_id)], + }, + exact=True, + ) + self.assertIsNone(response_body["rooms"][room_id1].get("invite_state")) + def test_rooms_required_state_expand_lazy_loading_room_members_incremental_sync( self, ) -> None: @@ -1243,7 +1329,7 @@ def test_rooms_required_state_expand_retract_expand(self) -> None: # Update the room name self.helper.send_state( - room_id1, "m.room.name", {"name": "Bar"}, state_key="", tok=user1_tok + room_id1, EventTypes.Name, {"name": "Bar"}, state_key="", tok=user1_tok ) # Update the sliding sync requests to exclude the room name again From 347743f199843569c1cf4ccdb8d0d0128088cf3b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 21 Nov 2024 13:24:51 -0600 Subject: [PATCH 2/7] Include membership change targets when lazy-loading room membership --- synapse/handlers/sliding_sync/__init__.py | 21 ++-- synapse/types/handlers/sliding_sync.py | 11 +- .../sliding_sync/test_rooms_required_state.py | 108 +++++++++++++++--- 3 files changed, 108 insertions(+), 32 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index 46a41d6623c..c2250e9c3d3 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -956,16 +956,6 @@ async def get_room_sync_data( ): lazy_load_room_members = True - # For incremental syncs that aren't limited, when - # lazy-loading room members, also include any membership - # that has changed. This allows clients to cache the - # membership list for as long as it doesn't get a gappy - # sync, but still ensures for large gaps the server doesn't - # need to send down all membership changes. - # if not initial and not limited: - # # `None` is a wildcard in the `StateFilter` - # required_state_types.append((EventTypes.Member, None)) - # Everyone in the timeline is relevant # # FIXME: We probably also care about invite, ban, kick, targets, etc @@ -973,8 +963,19 @@ async def get_room_sync_data( timeline_membership: Set[str] = set() if timeline_events is not None: for timeline_event in timeline_events: + # Anyone who sent a message is relevant timeline_membership.add(timeline_event.sender) + # We also care about invite, ban, kick, targets, + # etc. This allows clients to cache the membership + # list for as long as it doesn't get a gappy sync, + # but still ensures for large gaps the server + # doesn't need to send down all membership changes. + if timeline_event.type == EventTypes.Member: + timeline_membership.add( + timeline_event.state_key + ) + # Update the required state filter so we pick up the new # membership for user_id in timeline_membership: diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index aae60fddeab..3ebd334a6d5 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -407,8 +407,8 @@ class StateValues: # Include all state events of the given type WILDCARD: Final = "*" # Lazy-load room membership events (include room membership events for any event - # `sender` in the timeline). We only give special meaning to this value when it's a - # `state_key`. + # `sender` or membership change target in the timeline). We only give special + # meaning to this value when it's a `state_key`. LAZY: Final = "$LAZY" # Subsitute with the requester's user ID. Typically used by clients to get # the user's membership. @@ -641,9 +641,10 @@ def must_await_full_state( if user_id == StateValues.ME: continue # We're lazy-loading membership so we can just return the state we have. - # Lazy-loading means we include membership for any event `sender` in the - # timeline but since we had to auth those timeline events, we will have the - # membership state for them (including from remote senders). + # Lazy-loading means we include membership for any event `sender` or + # membership change target in the timeline but since we had to auth those + # timeline events, we will have the membership state for them (including + # from remote senders). elif user_id == StateValues.LAZY: continue elif user_id == StateValues.WILDCARD: diff --git a/tests/rest/client/sliding_sync/test_rooms_required_state.py b/tests/rest/client/sliding_sync/test_rooms_required_state.py index a7f06e15619..b4869d5fa3c 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -11,6 +11,7 @@ # See the GNU Affero General Public License for more details: # . # +import enum import logging from parameterized import parameterized, parameterized_class @@ -18,9 +19,9 @@ from twisted.test.proto_helpers import MemoryReactor import synapse.rest.admin -from synapse.api.constants import EventTypes, Membership, EventContentFields +from synapse.api.constants import EventContentFields, EventTypes, JoinRules, Membership from synapse.handlers.sliding_sync import StateValues -from synapse.rest.client import login, room, sync +from synapse.rest.client import knock, login, room, sync from synapse.server import HomeServer from synapse.util import Clock @@ -30,6 +31,17 @@ logger = logging.getLogger(__name__) +# Inherit from `str` so that they show up in the test description when we +# `@parameterized.expand(...)` the first parameter +class MembershipAction(str, enum.Enum): + INVITE = "invite" + JOIN = "join" + KNOCK = "knock" + LEAVE = "leave" + BAN = "ban" + KICK = "kick" + + # FIXME: This can be removed once we bump `SCHEMA_COMPAT_VERSION` and run the # foreground update for # `sliding_sync_joined_rooms`/`sliding_sync_membership_snapshots` (tracked by @@ -52,6 +64,7 @@ class SlidingSyncRoomsRequiredStateTestCase(SlidingSyncBase): servlets = [ synapse.rest.admin.register_servlets, login.register_servlets, + knock.register_servlets, room.register_servlets, sync.register_servlets, ] @@ -496,8 +509,19 @@ def test_rooms_required_state_lazy_loading_room_members_incremental_sync( ) self.assertIsNone(response_body["rooms"][room_id1].get("invite_state")) + @parameterized.expand( + [ + (MembershipAction.LEAVE,), + (MembershipAction.INVITE,), + (MembershipAction.KNOCK,), + (MembershipAction.JOIN,), + (MembershipAction.BAN,), + (MembershipAction.KICK,), + ] + ) def test_rooms_required_state_changed_membership_in_timeline_lazy_loading_room_members_incremental_sync( self, + room_membership_action: str, ) -> None: """ On incremental sync, test `rooms.required_state` returns people relevant to the @@ -515,12 +539,32 @@ def test_rooms_required_state_changed_membership_in_timeline_lazy_loading_room_m user5_id = self.register_user("user5", "pass") user5_tok = self.login(user5_id, "pass") - room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok) + room_id1 = self.helper.create_room_as(user2_id, tok=user2_tok, is_public=True) + # If we're testing knocks, set the room to knock + if room_membership_action == MembershipAction.KNOCK: + self.helper.send_state( + room_id1, + EventTypes.JoinRules, + {"join_rule": JoinRules.KNOCK}, + tok=user2_tok, + ) + + # Join the test users to the room + self.helper.invite(room_id1, src=user2_id, targ=user1_id, tok=user2_tok) self.helper.join(room_id1, user1_id, tok=user1_tok) + self.helper.invite(room_id1, src=user2_id, targ=user3_id, tok=user2_tok) self.helper.join(room_id1, user3_id, tok=user3_tok) + self.helper.invite(room_id1, src=user2_id, targ=user4_id, tok=user2_tok) self.helper.join(room_id1, user4_id, tok=user4_tok) - self.helper.join(room_id1, user5_id, tok=user5_tok) - + if room_membership_action in ( + MembershipAction.LEAVE, + MembershipAction.BAN, + MembershipAction.JOIN, + ): + self.helper.invite(room_id1, src=user2_id, targ=user5_id, tok=user2_tok) + self.helper.join(room_id1, user5_id, tok=user5_tok) + + # Send some messages to fill up the space self.helper.send(room_id1, "1", tok=user2_tok) self.helper.send(room_id1, "2", tok=user2_tok) self.helper.send(room_id1, "3", tok=user2_tok) @@ -543,18 +587,48 @@ def test_rooms_required_state_changed_membership_in_timeline_lazy_loading_room_m # Send more timeline events into the room self.helper.send(room_id1, "4", tok=user2_tok) self.helper.send(room_id1, "5", tok=user4_tok) - # self.helper.send(room_id1, "6", tok=user4_tok) - # Update the display name of user5 (causing a membership change) - self.helper.send_state( - room_id1, - event_type=EventTypes.Member, - state_key=user5_id, - body={ - EventContentFields.MEMBERSHIP: "join", - EventContentFields.MEMBERSHIP_DISPLAYNAME: "quick changer", - }, - tok=user5_tok, - ) + # The third event will be our membership event concerning user5 + if room_membership_action == MembershipAction.LEAVE: + # User 5 leaves + self.helper.leave(room_id1, user5_id, tok=user5_tok) + elif room_membership_action == MembershipAction.INVITE: + # User 5 is invited + self.helper.invite(room_id1, src=user2_id, targ=user5_id, tok=user2_tok) + elif room_membership_action == MembershipAction.KNOCK: + # User 5 knocks + self.helper.knock(room_id1, user5_id, tok=user5_tok) + # The admin of the room accepts the knock + self.helper.invite(room_id1, src=user2_id, targ=user5_id, tok=user2_tok) + elif room_membership_action == MembershipAction.JOIN: + # Update the display name of user5 (causing a membership change) + self.helper.send_state( + room_id1, + event_type=EventTypes.Member, + state_key=user5_id, + body={ + EventContentFields.MEMBERSHIP: Membership.JOIN, + EventContentFields.MEMBERSHIP_DISPLAYNAME: "quick changer", + }, + tok=user5_tok, + ) + elif room_membership_action == MembershipAction.BAN: + self.helper.ban(room_id1, src=user2_id, targ=user5_id, tok=user2_tok) + elif room_membership_action == MembershipAction.KICK: + # Kick user5 from the room + self.helper.change_membership( + room=room_id1, + src=user2_id, + targ=user5_id, + tok=user2_tok, + membership=Membership.LEAVE, + extra_data={ + "reason": "Bad manners", + }, + ) + else: + raise AssertionError( + f"Unknown room_membership_action: {room_membership_action}" + ) # Make an incremental Sliding Sync request response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) From 5707dde276d80caf0cb1e16605059e375145cadb Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 21 Nov 2024 13:25:58 -0600 Subject: [PATCH 3/7] Remove todo as we did it --- synapse/handlers/sliding_sync/__init__.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index c2250e9c3d3..dfc49815048 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -957,9 +957,6 @@ async def get_room_sync_data( lazy_load_room_members = True # Everyone in the timeline is relevant - # - # FIXME: We probably also care about invite, ban, kick, targets, etc - # but the spec only mentions "senders". timeline_membership: Set[str] = set() if timeline_events is not None: for timeline_event in timeline_events: From 5d1bd3173f2f153daf5630fd1026c4a33c31a34a Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 21 Nov 2024 13:29:08 -0600 Subject: [PATCH 4/7] Add changelog --- changelog.d/17947.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17947.feature diff --git a/changelog.d/17947.feature b/changelog.d/17947.feature new file mode 100644 index 00000000000..2d1b99cec2d --- /dev/null +++ b/changelog.d/17947.feature @@ -0,0 +1 @@ +Update [MSC4186](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) Sliding Sync to include invite, ban, kick, targets when `$LAZY`-loading room members. From 1b30fba58a054e5d4433e5445f0e57673dae9dba Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 21 Nov 2024 13:54:21 -0600 Subject: [PATCH 5/7] Update changelog to describe user-facing change --- changelog.d/17947.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/17947.feature b/changelog.d/17947.feature index 2d1b99cec2d..e5f8094016c 100644 --- a/changelog.d/17947.feature +++ b/changelog.d/17947.feature @@ -1 +1 @@ -Update [MSC4186](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) Sliding Sync to include invite, ban, kick, targets when `$LAZY`-loading room members. +Update [MSC4186](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) Sliding Sync to include invite, ban, kick, targets when `$LAZY`-loading room members to ensure clients get all membership updates for non-gappy syncs. From f92e171ea46afef34bb8b04c34b0dcf3855f385f Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 2 Dec 2024 09:42:31 -0600 Subject: [PATCH 6/7] Update changelog Co-authored-by: Erik Johnston --- changelog.d/17947.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/17947.feature b/changelog.d/17947.feature index e5f8094016c..2d1b99cec2d 100644 --- a/changelog.d/17947.feature +++ b/changelog.d/17947.feature @@ -1 +1 @@ -Update [MSC4186](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) Sliding Sync to include invite, ban, kick, targets when `$LAZY`-loading room members to ensure clients get all membership updates for non-gappy syncs. +Update [MSC4186](https://github.com/matrix-org/matrix-spec-proposals/pull/4186) Sliding Sync to include invite, ban, kick, targets when `$LAZY`-loading room members. From 3ab2d8d44a83e1b3546f575bdaee188080840f21 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Mon, 2 Dec 2024 09:51:58 -0600 Subject: [PATCH 7/7] Update synapse/handlers/sliding_sync/__init__.py Co-authored-by: Erik Johnston --- synapse/handlers/sliding_sync/__init__.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/synapse/handlers/sliding_sync/__init__.py b/synapse/handlers/sliding_sync/__init__.py index dfc49815048..4f4faef524a 100644 --- a/synapse/handlers/sliding_sync/__init__.py +++ b/synapse/handlers/sliding_sync/__init__.py @@ -964,10 +964,7 @@ async def get_room_sync_data( timeline_membership.add(timeline_event.sender) # We also care about invite, ban, kick, targets, - # etc. This allows clients to cache the membership - # list for as long as it doesn't get a gappy sync, - # but still ensures for large gaps the server - # doesn't need to send down all membership changes. + # etc. if timeline_event.type == EventTypes.Member: timeline_membership.add( timeline_event.state_key