Handle more edge cases

synapse/handlers/sliding_sync.py

@@ -21,6 +21,15 @@ if TYPE_CHECKING:
 logger = logging.getLogger(__name__)
 
 
+# Everything except `Membership.LEAVE`
+MEMBERSHIP_TO_DISPLAY_IN_SYNC = (
+    Membership.INVITE,
+    Membership.JOIN,
+    Membership.KNOCK,
+    Membership.BAN,
+)
+
+
 class SlidingSyncConfig(SlidingSyncBody):
     user: UserID
     device_id: str
@@ -213,36 +222,42 @@ class SlidingSyncHandler:
         """
         user_id = user.to_string()
 
-        room_for_user_list = await self.store.get_rooms_for_local_user_where_membership_is(
+        # 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,
-            # List everything except `Membership.LEAVE`
-            membership_list=(
-                Membership.INVITE,
-                Membership.JOIN,
-                Membership.KNOCK,
-                Membership.BAN,
-            ),
+                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 {}
+
+        # Our working list of rooms that can show up in the sync response
+        sync_room_id_set = {
+            room_for_user.room_id
+            for room_for_user in room_for_user_list
+            if room_for_user.membership in MEMBERSHIP_TO_DISPLAY_IN_SYNC
+        }
+
+        # Find the stream_ordering of the latest room membership event for the user
+        # which will mark the spot we queried up to.
         max_stream_ordering_from_room_list = max(
             room_for_user.stream_ordering for room_for_user in room_for_user_list
         )
 
-        # Our working list of rooms that can show up in the sync response
-        sync_room_id_set = {
-            room_for_user.room_id for room_for_user in room_for_user_list
-        }
-
         # If our `to_token` is already the same or ahead of the latest room membership
-        # for the user, we can just straight up return the room list (nothing has
+        # for the user, we can just straight-up return the room list (nothing has
         # changed)
         if max_stream_ordering_from_room_list <= to_token.room_key.stream:
             return sync_room_id_set
 
-        # We assume the `from_token` is before the `to_token`
+        # We assume the `from_token` is before or at-least equal to the `to_token`
         assert (
-            from_token is None or from_token.room_key.stream < to_token.room_key.stream
-        ), f"{from_token.room_key.stream if from_token else None} < {to_token.room_key.stream}"
+            from_token is None or from_token.room_key.stream <= to_token.room_key.stream
+        ), f"{from_token.room_key.stream if from_token else None} <= {to_token.room_key.stream}"
 
         # We assume the `from_token`/`to_token` is before the `max_stream_ordering_from_room_list`
         assert (
@@ -253,7 +268,7 @@ class SlidingSyncHandler:
             to_token.room_key.stream < max_stream_ordering_from_room_list
         ), f"{to_token.room_key.stream} < {max_stream_ordering_from_room_list}"
 
-        # Since we fetched the users room list at some point in time after the to/from
+        # Since we fetched the users room list at some point in time after the from/to
         # tokens, we need to revert some membership changes to match the point in time
         # of the `to_token`.
         #
@@ -262,7 +277,8 @@ class SlidingSyncHandler:
         # - 2b) Add back rooms that the user left after the `to_token`
         membership_change_events = await self.store.get_membership_changes_for_user(
             user_id,
-            # Start from the `from_token` if given, otherwise from the `to_token`
+            # Start from the `from_token` if given, otherwise from the `to_token` so we
+            # can still do the 2) fixups.
             from_key=from_token.room_key if from_token else to_token.room_key,
             to_key=RoomStreamToken(stream=max_stream_ordering_from_room_list),
             excluded_rooms=self.rooms_to_exclude_globally,
@@ -270,7 +286,7 @@ class SlidingSyncHandler:
 
         # 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.
+        # 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] = {}
         for event in membership_change_events:

tests/handlers/test_sliding_sync.py

@@ -32,7 +32,30 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
         self.store = self.hs.get_datastores().main
         self.event_sources = hs.get_event_sources()
 
+    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, {})
+
     def test_get_newly_joined_room(self) -> None:
+        """
+        Test that rooms that the user has newly_joined show up. newly_joined is when you
+        join after the `from_token` and <= `to_token`.
+        """
         user1_id = self.register_user("user1", "pass")
         user1_tok = self.login(user1_id, "pass")
 
@@ -53,6 +76,9 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
         self.assertEqual(room_id_results, {room_id})
 
     def test_get_already_joined_room(self) -> None:
+        """
+        Test that rooms that the user is already joined show up.
+        """
         user1_id = self.register_user("user1", "pass")
         user1_tok = self.login(user1_id, "pass")
 
@@ -71,6 +97,10 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
         self.assertEqual(room_id_results, {room_id})
 
     def test_get_invited_banned_knocked_room(self) -> None:
+        """
+        Test that rooms that the user is 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")
@@ -129,6 +159,11 @@ 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
+        the `get_sync_room_ids_for_user` method.
+        """
         user1_id = self.register_user("user1", "pass")
         user1_tok = self.login(user1_id, "pass")
 
@@ -136,20 +171,105 @@ class GetSyncRoomIdsForUserTestCase(HomeserverTestCase):
         room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok)
         self.helper.leave(room_id1, user1_id, tok=user1_tok)
 
-        before_room_token = self.event_sources.get_current_token()
+        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)
         self.helper.leave(room_id1, user1_id, tok=user1_tok)
 
-        after_room_token = self.event_sources.get_current_token()
+        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=before_room_token,
-                to_token=after_room_token,
+                from_token=after_room1_token,
+                to_token=after_room2_token,
             )
         )
 
+        # Only the newly_left room should show up
         self.assertEqual(room_id_results, {room_id2})
+
+    def test_no_joins_after_to_token(self) -> None:
+        """
+        Rooms we join after the `to_token` should not 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")
+
+        before_room1_token = self.event_sources.get_current_token()
+
+        room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok)
+
+        after_room1_token = self.event_sources.get_current_token()
+
+        # Room join after after our `to_token` shouldn't show up
+        room_id2 = self.helper.create_room_as(user1_id, tok=user1_tok)
+        _ = room_id2
+
+        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,
+            )
+        )
+
+        self.assertEqual(room_id_results, {room_id1})
+
+    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.
+        """
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+
+        before_room1_token = self.event_sources.get_current_token()
+
+        room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok)
+
+        after_room1_token = self.event_sources.get_current_token()
+
+        # Leave the room after we already have our tokens
+        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,
+            )
+        )
+
+        # We should still see the room because we were joined during the
+        # from_token/to_token time period.
+        self.assertEqual(room_id_results, {room_id1})
+
+    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
+        """
+        user1_id = self.register_user("user1", "pass")
+        user1_tok = self.login(user1_id, "pass")
+
+        room_id1 = self.helper.create_room_as(user1_id, tok=user1_tok)
+
+        after_room1_token = self.event_sources.get_current_token()
+
+        # Leave the room after we already have our tokens
+        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,
+            )
+        )
+
+        # We should still see the room because we were joined during the
+        # from_token/to_token time period.
+        self.assertEqual(room_id_results, {room_id1})