diff --git a/synapse/handlers/sliding_sync.py b/synapse/handlers/sliding_sync.py index 7ea55bb230..ea3fc32c54 100644 --- a/synapse/handlers/sliding_sync.py +++ b/synapse/handlers/sliding_sync.py @@ -222,18 +222,22 @@ class SlidingSyncHandler: """ user_id = user.to_string() - # First grab the current rooms for the user - room_for_user_list = ( - await self.store.get_rooms_for_local_user_where_membership_is( - user_id=user_id, - membership_list=Membership.LIST, - excluded_rooms=self.rooms_to_exclude_globally, - ) + # First grab a current snapshot rooms for the user + room_for_user_list = await self.store.get_rooms_for_local_user_where_membership_is( + user_id=user_id, + # We want to fetch any kind of membership (joined and left rooms) in order + # to get the `stream_ordering` of the latest room membership event for the + # user. + # + # We will filter out the rooms that the user has left below (see + # `MEMBERSHIP_TO_DISPLAY_IN_SYNC`) + membership_list=Membership.LIST, + excluded_rooms=self.rooms_to_exclude_globally, ) # If the user has never joined any rooms before, we can just return an empty list if not room_for_user_list: - return {} + return set() # Our working list of rooms that can show up in the sync response sync_room_id_set = { @@ -284,11 +288,23 @@ class SlidingSyncHandler: excluded_rooms=self.rooms_to_exclude_globally, ) + logger.info( + f"> from_token={from_token.room_key.stream} <= to_token={to_token.room_key.stream}, max_stream_ordering_from_room_list={max_stream_ordering_from_room_list}" + ) + logger.info( + "membership_change_events: %s", + [ + f"{event.internal_metadata.stream_ordering}: {event.membership}" + for event in membership_change_events + ], + ) + # 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] = {} last_membership_change_by_room_id_after_to_token: Dict[str, EventBase] = {} + first_membership_change_by_room_id_after_to_token: Dict[str, EventBase] = {} for event in membership_change_events: assert event.internal_metadata.stream_ordering @@ -305,6 +321,9 @@ class SlidingSyncHandler: <= max_stream_ordering_from_room_list ): last_membership_change_by_room_id_after_to_token[event.room_id] = event + first_membership_change_by_room_id_after_to_token.setdefault( + event.room_id, event + ) else: raise AssertionError( "Membership event with stream_ordering=%s should fall in the given ranges above" @@ -316,6 +335,8 @@ class SlidingSyncHandler: max_stream_ordering_from_room_list, ) + logger.info("before fix-up: sync_room_id_set %s", sync_room_id_set) + # 1) for event in last_membership_change_by_room_id_in_from_to_range.values(): # 1) Add back newly left rooms (> `from_token` and <= `to_token`). We @@ -324,22 +345,58 @@ class SlidingSyncHandler: if event.membership == Membership.LEAVE: sync_room_id_set.add(event.room_id) + logger.info("after 1: sync_room_id_set %s", sync_room_id_set) + # 2) # TODO: Verify this logic is correct - for event in last_membership_change_by_room_id_after_to_token.values(): + for ( + last_membership_change_after_to_token + ) in last_membership_change_by_room_id_after_to_token.values(): + last_membership_change_in_from_to_range = ( + last_membership_change_by_room_id_in_from_to_range.get(event.room_id) + ) + first_membership_change_after_to_token = ( + first_membership_change_by_room_id_after_to_token.get(event.room_id) + ) + # 2a) Add back rooms that the user left after the `to_token` - if event.membership == Membership.LEAVE: - sync_room_id_set.add(event.room_id) + # + # If the last membership event after the `to_token` is a leave event, then + # the room was excluded from the + # `get_rooms_for_local_user_where_membership_is()` results. We should add + # these rooms back as long as the user was part of the room before the + # `to_token`. + if ( + last_membership_change_after_to_token.membership == Membership.LEAVE + and ( + # If the first/last event are the same, we can gurantee the user + # didn't join/leave multiple times after the `to_token` (meaning + # they were in the room before). + first_membership_change_after_to_token.event_id + == last_membership_change_after_to_token.event_id + or ( + # Or if the first/last event are different, then we need to make + # sure that the first event after the `to_token` is NOT a leave + # event (meaning they were in the room before). + first_membership_change_after_to_token.event_id + != last_membership_change_after_to_token.event_id + and first_membership_change_after_to_token.membership + != Membership.LEAVE + ) + ) + ): + sync_room_id_set.add(last_membership_change_after_to_token.room_id) # 2b) Remove rooms that the user joined (hasn't left) after the `to_token` elif ( - event.membership != Membership.LEAVE + last_membership_change_after_to_token.membership != Membership.LEAVE # We don't want to remove the the room if the user was still joined # before the `to_token`. - and last_membership_change_by_room_id_in_from_to_range.get( - event.room_id + and ( + last_membership_change_in_from_to_range is None + or last_membership_change_in_from_to_range.membership + == Membership.LEAVE ) - is None ): - sync_room_id_set.discard(event.room_id) + sync_room_id_set.discard(last_membership_change_after_to_token.room_id) return sync_room_id_set diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 21d3f1db06..0c94558dcc 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -49,7 +49,7 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): ) ) - self.assertEqual(room_id_results, {}) + self.assertEqual(room_id_results, set()) def test_get_newly_joined_room(self) -> None: """ @@ -220,8 +220,9 @@ 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 we were joined during the - from_token/to_token. + Room still shows up if we left the room but were joined during the + from_token/to_token. See condition "2b)" comments in the + `get_sync_room_ids_for_user()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -249,8 +250,9 @@ 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 we were joined before the - `from_token` so it should show up + Room still shows up if we left the room but were joined before the `from_token` + so it should show up. See condition "2b)" comments in the + `get_sync_room_ids_for_user()` method. """ user1_id = self.register_user("user1", "pass") user1_tok = self.login(user1_id, "pass") @@ -270,6 +272,187 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase): ) ) - # We should still see the room because we were joined during the - # from_token/to_token time period. + # We should still see the room because we were joined before the `from_token` + self.assertEqual(room_id_results, {room_id1}) + + 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. + """ + 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 and leave the room after we already have our tokens + self.helper.join(room_id1, user1_id, tok=user1_tok) + 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( + 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 + 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") + + # 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 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) + + after_room1_token = self.event_sources.get_current_token() + + # Join and leave the room after we already have our tokens + self.helper.join(room_id1, user1_id, tok=user1_tok) + 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( + 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, set()) + + def test_join_leave_multiple_times_during_range_and_after_to_token( + self, + ) -> None: + """ + TODO + """ + 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, leave, join back to 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) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + after_room1_token = self.event_sources.get_current_token() + + # Leave and Join the room multiple times after we already have our tokens + self.helper.leave(room_id1, user1_id, tok=user1_tok) + self.helper.join(room_id1, user1_id, tok=user1_tok) + 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( + UserID.from_string(user1_id), + from_token=before_room1_token, + to_token=after_room1_token, + ) + ) + + # Room should show up because it was newly_left and joined during the from/to range + self.assertEqual(room_id_results, {room_id1}) + + def test_join_leave_multiple_times_before_range_and_after_to_token( + self, + ) -> None: + """ + TODO + """ + 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") + + # 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 + self.helper.join(room_id1, user1_id, tok=user1_tok) + self.helper.leave(room_id1, user1_id, tok=user1_tok) + self.helper.join(room_id1, user1_id, tok=user1_tok) + + after_room1_token = self.event_sources.get_current_token() + + # Leave and Join the room multiple times after we already have our tokens + self.helper.leave(room_id1, user1_id, tok=user1_tok) + self.helper.join(room_id1, user1_id, tok=user1_tok) + 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( + UserID.from_string(user1_id), + from_token=after_room1_token, + to_token=after_room1_token, + ) + ) + + # Room should show up because we were joined before the from/to range + self.assertEqual(room_id_results, {room_id1}) + + def test_TODO( + self, + ) -> None: + """ + TODO + """ + 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") + + # 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) + + self.helper.invite(room_id1, src=user2_id, targ=user1_id, tok=user2_tok) + + after_room1_token = self.event_sources.get_current_token() + + # Leave and Join the room multiple times after we already have our tokens + self.helper.join(room_id1, user1_id, tok=user1_tok) + 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( + UserID.from_string(user1_id), + from_token=after_room1_token, + to_token=after_room1_token, + ) + ) + + # Room should show up because we were invited before the from/to range self.assertEqual(room_id_results, {room_id1})