From dfee21a1f4d068077fc0fb8beb0c7d005faa507b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Tue, 4 Jun 2024 18:29:14 -0500 Subject: [PATCH] Switch fixup order to fix edge case with newly_left rooms Previously, we added back newly_left rooms and our second fixup was removing them. We can just switch the order of the fixups to solve this. --- synapse/handlers/sliding_sync.py | 86 +++++++++++++++-------------- tests/handlers/test_sliding_sync.py | 56 +++++++++++++++---- 2 files changed, 90 insertions(+), 52 deletions(-) diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 863e66ca3b..429d4eb192 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -480,9 +480,9 @@ class SlidingSyncHandler: # 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: # - # - 1) Add back newly_left rooms (> `from_token` and <= `to_token`) - # - 2a) Remove rooms that the user joined after the `to_token` - # - 2b) Add back rooms that the user left after the `to_token` + # - 1a) Remove rooms that the user joined after the `to_token` + # - 1b) Add back rooms that the user left after the `to_token` + # - 2) Add back newly_left rooms (> `from_token` and <= `to_token`) # # Below, we're doing two separate lookups for membership changes. We could # request everything for both fixups in one range, [`from_token.room_key`, @@ -491,43 +491,9 @@ class SlidingSyncHandler: # `event.internal_metadata` to include `instance_name` but it might turn out a # little difficult and a bigger, broader Synapse change than we want to make. - # 1) ----------------------------------------------------- - - # 1) Fetch membership changes that fall in the range from `from_token` up to `to_token` - membership_change_events_in_from_to_range = [] - if from_token: - membership_change_events_in_from_to_range = ( - await self.store.get_membership_changes_for_user( - user_id, - from_key=from_token.room_key, - to_key=to_token.room_key, - excluded_rooms=self.rooms_to_exclude_globally, - ) - ) - - # 1) Assemble a list of the last membership events in some given ranges. Someone - # could have left and joined multiple times during the given range but we only - # care about end-result so we grab the last one. - last_membership_change_by_room_id_in_from_to_range: Dict[str, EventBase] = {} - for event in membership_change_events_in_from_to_range: - assert event.internal_metadata.stream_ordering - last_membership_change_by_room_id_in_from_to_range[event.room_id] = event - - # 1) Fixup - for ( - last_membership_change_in_from_to_range - ) in last_membership_change_by_room_id_in_from_to_range.values(): - room_id = last_membership_change_in_from_to_range.room_id - - # 1) 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.add(room_id) - # 2) ----------------------------------------------------- - # 2) Fetch membership changes that fall in the range from `to_token` up to + # 1) Fetch membership changes that fall in the range from `to_token` up to # `membership_snapshot_token` membership_change_events_after_to_token = ( await self.store.get_membership_changes_for_user( @@ -538,7 +504,7 @@ class SlidingSyncHandler: ) ) - # 2) Assemble a list of the last membership events in some given ranges. Someone + # 1) Assemble a list of the last membership events in some given ranges. Someone # could have left and joined multiple times during the given range but we only # care about end-result so we grab the last one. last_membership_change_by_room_id_after_to_token: Dict[str, EventBase] = {} @@ -554,7 +520,7 @@ class SlidingSyncHandler: event.room_id, event ) - # 2) Fixup + # 1) Fixup for ( last_membership_change_after_to_token ) in last_membership_change_by_room_id_after_to_token.values(): @@ -611,7 +577,7 @@ class SlidingSyncHandler: sender=last_membership_change_after_to_token.sender, ) - # 2a) Add back rooms that the user left after the `to_token` + # 1a) Add back rooms that the user left after the `to_token` # # For example, if the last membership event after the `to_token` is a leave # event, then the room was excluded from `sync_room_id_set` when we first @@ -622,7 +588,7 @@ class SlidingSyncHandler: and should_prev_membership_be_included ): sync_room_id_set.add(room_id) - # 2b) Remove rooms that the user joined (hasn't left) after the `to_token` + # 1b) Remove rooms that the user joined (hasn't left) after the `to_token` # # For example, if the last membership event after the `to_token` is a "join" # event, then the room was included `sync_room_id_set` when we first crafted @@ -634,4 +600,40 @@ class SlidingSyncHandler: ): sync_room_id_set.discard(room_id) + # 2) ----------------------------------------------------- + # We fix-up newly_left rooms after the first fixup because it may have removed + # some left rooms that we can figure out our newly_left in the following code + + # 2) Fetch membership changes that fall in the range from `from_token` up to `to_token` + membership_change_events_in_from_to_range = [] + if from_token: + membership_change_events_in_from_to_range = ( + await self.store.get_membership_changes_for_user( + user_id, + from_key=from_token.room_key, + to_key=to_token.room_key, + excluded_rooms=self.rooms_to_exclude_globally, + ) + ) + + # 2) Assemble a list of the last membership events in some given ranges. Someone + # could have left and joined multiple times during the given range but we only + # care about end-result so we grab the last one. + last_membership_change_by_room_id_in_from_to_range: Dict[str, EventBase] = {} + for event in membership_change_events_in_from_to_range: + assert event.internal_metadata.stream_ordering + last_membership_change_by_room_id_in_from_to_range[event.room_id] = event + + # 2) Fixup + for ( + last_membership_change_in_from_to_range + ) in last_membership_change_by_room_id_in_from_to_range.values(): + room_id = last_membership_change_in_from_to_range.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: + sync_room_id_set.add(room_id) + return sync_room_id_set diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 29a963000a..53e6ebb488 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -305,7 +305,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): 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 "1)" comments in + were left before the `from_token` don't show up. See condition "2)" comments in the `get_sync_room_ids_for_user` method. """ user1_id = self.register_user("user1", "pass") @@ -336,7 +336,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): def test_no_joins_after_to_token(self) -> None: """ - Rooms we join after the `to_token` should *not* show up. See condition "2b)" + Rooms we join after the `to_token` should *not* show up. See condition "1b)" comments in the `get_sync_room_ids_for_user()` method. """ user1_id = self.register_user("user1", "pass") @@ -365,7 +365,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): 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 "2a)" comments in the + from_token/to_token. See condition "1a)" comments in the `get_sync_room_ids_for_user()` method. """ user1_id = self.register_user("user1", "pass") @@ -395,7 +395,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): 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 "2a)" comments in the + so it should show up. See condition "1a)" comments in the `get_sync_room_ids_for_user()` method. """ user1_id = self.register_user("user1", "pass") @@ -422,7 +422,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): 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 "2a)" comments in the + so it should show up. See condition "1a)" comments in the `get_sync_room_ids_for_user()` method. """ user1_id = self.register_user("user1", "pass") @@ -470,8 +470,8 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): 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 "2a)" comments - in the `get_sync_room_ids_for_user()` method. + 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. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -504,10 +504,46 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): # Room should still show up because it's newly_left during the from/to range self.assertEqual(room_id_results, {room_id1}) + 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. + """ + 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_room1_token = self.event_sources.get_current_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 and leave the room during the from/to range + self.helper.join(room_id1, user1_id, tok=user1_tok) + self.helper.leave(room_id1, user1_id, tok=user1_tok) + + after_room1_token = self.event_sources.get_current_token() + + # Join the room after we already have our tokens + 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( + UserID.from_string(user1_id), + from_token=before_room1_token, + to_token=after_room1_token, + ) + ) + + # Room should still show up because it's newly_left during the from/to range + self.assertEqual(room_id_results, {room_id1}) + 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 "2a)" comments + after the `to_token` doesn't mess with the results. See condition "1a)" comments in the `get_sync_room_ids_for_user()` method. """ user1_id = self.register_user("user1", "pass") @@ -542,7 +578,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): 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 "2b)" comments in the + `to_token` doesn't mess with the results. See condition "1b)" comments in the `get_sync_room_ids_for_user()` method. """ user1_id = self.register_user("user1", "pass") @@ -660,7 +696,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): ) -> None: """ 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 "2a)" comments in + from/to range so the room should still show up. See condition "1a)" comments in the `get_sync_room_ids_for_user()` method. """ user1_id = self.register_user("user1", "pass")