Merge remote-tracking branch 'origin/develop' into erikj/ss_remove_list_ops

changelog.d/17671.misc

@@ -0,0 +1 @@
+Remove unspecced list operations from the sliding sync response.

synapse/handlers/sliding_sync/extensions.py

@@ -20,6 +20,7 @@ from typing_extensions import assert_never
 
 from synapse.api.constants import AccountDataTypes, EduTypes
 from synapse.handlers.receipts import ReceiptEventSource
+from synapse.handlers.sliding_sync.room_lists import SlidingSyncListResult
 from synapse.logging.opentracing import trace
 from synapse.storage.databases.main.receipts import ReceiptInRoom
 from synapse.types import (
@@ -33,7 +34,6 @@ from synapse.types import (
 from synapse.types.handlers.sliding_sync import (
     HaveSentRoomFlag,
     MutablePerConnectionState,
-    OperationType,
     PerConnectionState,
     SlidingSyncConfig,
     SlidingSyncResult,
@@ -60,7 +60,7 @@ class SlidingSyncExtensionHandler:
         sync_config: SlidingSyncConfig,
         previous_connection_state: "PerConnectionState",
         new_connection_state: "MutablePerConnectionState",
-        actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
+        actual_lists: Mapping[str, SlidingSyncListResult],
         actual_room_ids: Set[str],
         actual_room_response_map: Mapping[str, SlidingSyncResult.RoomResult],
         to_token: StreamToken,
@@ -151,7 +151,7 @@ class SlidingSyncExtensionHandler:
         self,
         requested_lists: Optional[StrCollection],
         requested_room_ids: Optional[StrCollection],
-        actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
+        actual_lists: Mapping[str, SlidingSyncListResult],
         actual_room_ids: AbstractSet[str],
     ) -> Set[str]:
         """
@@ -193,28 +193,18 @@ class SlidingSyncExtensionHandler:
         if requested_lists is not None:
             for list_key in requested_lists:
                 # Just some typing because we share the variable name in multiple places
-                actual_list: Optional[SlidingSyncResult.SlidingWindowList] = None
+                actual_list: Optional[SlidingSyncListResult] = None
 
                 # A wildcard means we process rooms from all lists
                 if list_key == "*":
                     for actual_list in actual_lists.values():
-                        # We only expect a single SYNC operation for any list
-                        assert len(actual_list.ops) == 1
-                        sync_op = actual_list.ops[0]
-                        assert sync_op.op == OperationType.SYNC
-
-                        relevant_room_ids.update(sync_op.room_ids)
+                        relevant_room_ids.update(actual_list.room_ids)
 
                     break
 
                 actual_list = actual_lists.get(list_key)
                 if actual_list is not None:
-                    # We only expect a single SYNC operation for any list
-                    assert len(actual_list.ops) == 1
-                    sync_op = actual_list.ops[0]
-                    assert sync_op.op == OperationType.SYNC
-
-                    relevant_room_ids.update(sync_op.room_ids)
+                    relevant_room_ids.update(actual_list.room_ids)
 
         return relevant_room_ids
 
@@ -348,7 +338,7 @@ class SlidingSyncExtensionHandler:
     async def get_account_data_extension_response(
         self,
         sync_config: SlidingSyncConfig,
-        actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
+        actual_lists: Mapping[str, SlidingSyncListResult],
         actual_room_ids: Set[str],
         account_data_request: SlidingSyncConfig.Extensions.AccountDataExtension,
         to_token: StreamToken,
@@ -441,7 +431,7 @@ class SlidingSyncExtensionHandler:
         sync_config: SlidingSyncConfig,
         previous_connection_state: "PerConnectionState",
         new_connection_state: "MutablePerConnectionState",
-        actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
+        actual_lists: Mapping[str, SlidingSyncListResult],
         actual_room_ids: Set[str],
         actual_room_response_map: Mapping[str, SlidingSyncResult.RoomResult],
         receipts_request: SlidingSyncConfig.Extensions.ReceiptsExtension,
@@ -637,7 +627,7 @@ class SlidingSyncExtensionHandler:
     async def get_typing_extension_response(
         self,
         sync_config: SlidingSyncConfig,
-        actual_lists: Mapping[str, SlidingSyncResult.SlidingWindowList],
+        actual_lists: Mapping[str, SlidingSyncListResult],
         actual_room_ids: Set[str],
         actual_room_response_map: Mapping[str, SlidingSyncResult.RoomResult],
         typing_request: SlidingSyncConfig.Extensions.TypingExtension,

synapse/handlers/sliding_sync/room_lists.py

@@ -64,7 +64,6 @@ from synapse.types import (
 )
 from synapse.types.handlers.sliding_sync import (
     HaveSentRoomFlag,
-    OperationType,
     PerConnectionState,
     RoomSyncConfig,
     SlidingSyncConfig,
@@ -106,7 +105,7 @@ class SlidingSyncInterestedRooms:
         dm_room_ids: The set of rooms the user consider as direct-message (DM) rooms
     """
 
-    lists: Mapping[str, SlidingSyncResult.SlidingWindowList]
+    lists: Mapping[str, "SlidingSyncListResult"]
     relevant_room_map: Mapping[str, RoomSyncConfig]
     relevant_rooms_to_send_map: Mapping[str, RoomSyncConfig]
     all_rooms: Set[str]
@@ -117,6 +116,17 @@ class SlidingSyncInterestedRooms:
     dm_room_ids: AbstractSet[str]
 
 
+@attr.s(auto_attribs=True, slots=True, frozen=True)
+class SlidingSyncListResult(SlidingSyncResult.SlidingWindowList):
+    """Add a new field to the lists response that allows us to track the
+    matching room IDs in the list.
+
+    This is not returend to the client.
+    """
+
+    room_ids: StrCollection
+
+
 class Sentinel(enum.Enum):
     # defining a sentinel in this way allows mypy to correctly handle the
     # type of a dictionary lookup and subsequent type narrowing.
@@ -212,7 +222,7 @@ class SlidingSyncRoomLists:
         user_id = sync_config.user.to_string()
 
         # Assemble sliding window lists
-        lists: Dict[str, SlidingSyncResult.SlidingWindowList] = {}
+        lists: Dict[str, SlidingSyncListResult] = {}
         # Keep track of the rooms that we can display and need to fetch more info about
         relevant_room_map: Dict[str, RoomSyncConfig] = {}
         # The set of room IDs of all rooms that could appear in any list. These
@@ -350,7 +360,7 @@ class SlidingSyncRoomLists:
 
                     all_rooms.update(filtered_sync_room_map)
 
-                    ops: List[SlidingSyncResult.SlidingWindowList.Operation] = []
+                    room_ids_in_list: List[str] = []
 
                     if list_config.ranges:
                         if list_config.ranges == [(0, len(filtered_sync_room_map) - 1)]:
@@ -363,8 +373,6 @@ class SlidingSyncRoomLists:
                             )
 
                         for range in list_config.ranges:
-                            room_ids_in_list: List[str] = []
-
                             # We're going to loop through the sorted list of rooms starting
                             # at the range start index and keep adding rooms until we fill
                             # up the range or run out of rooms.
@@ -393,17 +401,8 @@ class SlidingSyncRoomLists:
 
                                 room_ids_in_list.append(room_id)
 
-                            ops.append(
-                                SlidingSyncResult.SlidingWindowList.Operation(
-                                    op=OperationType.SYNC,
-                                    range=range,
-                                    room_ids=room_ids_in_list,
-                                )
-                            )
-
-                    lists[list_key] = SlidingSyncResult.SlidingWindowList(
-                        count=len(sorted_room_info),
-                        ops=ops,
+                    lists[list_key] = SlidingSyncListResult(
+                        count=len(sorted_room_info), room_ids=room_ids_in_list
                     )
 
         if sync_config.room_subscriptions:
@@ -484,7 +483,7 @@ class SlidingSyncRoomLists:
         dm_room_ids = await self._get_dm_rooms_for_user(sync_config.user.to_string())
 
         # Assemble sliding window lists
-        lists: Dict[str, SlidingSyncResult.SlidingWindowList] = {}
+        lists: Dict[str, SlidingSyncListResult] = {}
         # Keep track of the rooms that we can display and need to fetch more info about
         relevant_room_map: Dict[str, RoomSyncConfig] = {}
         # The set of room IDs of all rooms that could appear in any list. These
@@ -535,11 +534,9 @@ class SlidingSyncRoomLists:
                         filtered_sync_room_map, to_token
                     )
 
-                    ops: List[SlidingSyncResult.SlidingWindowList.Operation] = []
+                    room_ids_in_list: List[str] = []
                     if list_config.ranges:
                         for range in list_config.ranges:
-                            room_ids_in_list: List[str] = []
-
                             # We're going to loop through the sorted list of rooms starting
                             # at the range start index and keep adding rooms until we fill
                             # up the range or run out of rooms.
@@ -568,17 +565,8 @@ class SlidingSyncRoomLists:
 
                                 room_ids_in_list.append(room_id)
 
-                            ops.append(
-                                SlidingSyncResult.SlidingWindowList.Operation(
-                                    op=OperationType.SYNC,
-                                    range=range,
-                                    room_ids=room_ids_in_list,
-                                )
-                            )
-
-                    lists[list_key] = SlidingSyncResult.SlidingWindowList(
-                        count=len(sorted_room_info),
-                        ops=ops,
+                    lists[list_key] = SlidingSyncListResult(
+                        count=len(sorted_room_info), room_ids=room_ids_in_list
                     )
 
         if sync_config.room_subscriptions:

synapse/rest/client/sync.py

@@ -792,14 +792,6 @@ class SlidingSyncRestServlet(RestServlet):
             "lists": {
                 "foo-list": {
                     "count": 1337,
-                    "ops": [{
-                        "op": "SYNC",
-                        "range": [0, 99],
-                        "room_ids": [
-                            "!foo:bar",
-                            // ... 99 more room IDs
-                        ]
-                    }]
                 }
             },
             // Aggregated rooms from lists and room subscriptions
@@ -977,20 +969,10 @@ class SlidingSyncRestServlet(RestServlet):
     def encode_lists(
         self, lists: Mapping[str, SlidingSyncResult.SlidingWindowList]
     ) -> JsonDict:
-        def encode_operation(
-            operation: SlidingSyncResult.SlidingWindowList.Operation,
-        ) -> JsonDict:
-            return {
-                "op": operation.op.value,
-                "range": operation.range,
-                "room_ids": operation.room_ids,
-            }
-
         serialized_lists = {}
         for list_key, list_result in lists.items():
             serialized_lists[list_key] = {
                 "count": list_result.count,
-                "ops": [encode_operation(op) for op in list_result.ops],
             }
 
         return serialized_lists

synapse/types/handlers/sliding_sync.py

@@ -30,7 +30,6 @@ from typing import (
     Optional,
     Sequence,
     Set,
-    Tuple,
     TypeVar,
     cast,
 )
@@ -225,27 +224,9 @@ class SlidingSyncResult:
         Attributes:
             count: The total number of entries in the list. Always present if this list
                 is.
-            ops: The sliding list operations to perform.
         """
 
-        @attr.s(slots=True, frozen=True, auto_attribs=True)
-        class Operation:
-            """
-            Attributes:
-                op: The operation type to perform.
-                range: Which index positions are affected by this operation. These are
-                    both inclusive.
-                room_ids: Which room IDs are affected by this operation. These IDs match
-                    up to the positions in the `range`, so the last room ID in this list
-                    matches the 9th index. The room data is held in a separate object.
-            """
-
-            op: OperationType
-            range: Tuple[int, int]
-            room_ids: List[str]
-
         count: int
-        ops: List[Operation]
 
     @attr.s(slots=True, frozen=True, auto_attribs=True)
     class Extensions:

tests/rest/client/sliding_sync/test_rooms_meta.py

@@ -926,18 +926,6 @@ class SlidingSyncRoomsMetaTestCase(SlidingSyncBase):
             response_body["lists"].keys(),
         )
 
-        # Make sure the list includes the rooms in the right order
-        self.assertEqual(
-            len(response_body["lists"]["foo-list"]["ops"]),
-            1,
-            response_body["lists"]["foo-list"],
-        )
-        op = response_body["lists"]["foo-list"]["ops"][0]
-        self.assertEqual(op["op"], "SYNC")
-        self.assertEqual(op["range"], [0, 1])
-        # Note that we don't order the ops anymore, so we need to compare sets.
-        self.assertIncludes(set(op["room_ids"]), {room_id1, room_id2}, exact=True)
-
         # The `bump_stamp` for room1 should point at the latest message (not the
         # reaction since it's not one of the `DEFAULT_BUMP_EVENT_TYPES`)
         self.assertEqual(

tests/rest/client/sliding_sync/test_rooms_required_state.py

@@ -735,14 +735,13 @@ class SlidingSyncRoomsRequiredStateTestCase(SlidingSyncBase):
         response_body, _ = self.do_sync(sync_body, tok=user1_tok)
 
         # The list should include both rooms now because we don't need full state
-        for list_key in response_body["lists"].keys():
-            self.assertIncludes(
-                set(response_body["lists"][list_key]["ops"][0]["room_ids"]),
-                {room_id2, room_id1},
-                exact=True,
-                message=f"Expected all rooms to show up for list_key={list_key}. Response "
-                + str(response_body["lists"][list_key]),
-            )
+        self.assertIncludes(
+            set(response_body["rooms"].keys()),
+            {room_id2, room_id1},
+            exact=True,
+            message="Expected all rooms to show up. Response "
+            + str(response_body["rooms"]),
+        )
 
         # Take each of the list variants and apply them to room subscriptions to make
         # sure the same rules apply
@@ -826,14 +825,13 @@ class SlidingSyncRoomsRequiredStateTestCase(SlidingSyncBase):
 
         # Make sure the list includes room1 but room2 is excluded because it's still
         # partially-stated
-        for list_key in response_body["lists"].keys():
-            self.assertIncludes(
-                set(response_body["lists"][list_key]["ops"][0]["room_ids"]),
-                {room_id1},
-                exact=True,
-                message=f"Expected only fully-stated rooms to show up for list_key={list_key}. Response "
-                + str(response_body["lists"][list_key]),
-            )
+        self.assertIncludes(
+            set(response_body["rooms"].keys()),
+            {room_id1},
+            exact=True,
+            message="Expected only fully-stated rooms to show up. Response "
+            + str(response_body["rooms"]),
+        )
 
         # Take each of the list variants and apply them to room subscriptions to make
         # sure the same rules apply

tests/rest/client/sliding_sync/test_sliding_sync.py

@@ -348,47 +348,6 @@ class SlidingSyncTestCase(SlidingSyncBase):
 
         return room_id
 
-    def test_sync_list(self) -> None:
-        """
-        Test that room IDs show up in the Sliding Sync `lists`
-        """
-        user1_id = self.register_user("user1", "pass")
-        user1_tok = self.login(user1_id, "pass")
-
-        room_id = self.helper.create_room_as(user1_id, tok=user1_tok, is_public=True)
-
-        # Make the Sliding Sync request
-        sync_body = {
-            "lists": {
-                "foo-list": {
-                    "ranges": [[0, 99]],
-                    "required_state": [],
-                    "timeline_limit": 1,
-                }
-            }
-        }
-        response_body, _ = self.do_sync(sync_body, tok=user1_tok)
-
-        # Make sure it has the foo-list we requested
-        self.assertListEqual(
-            list(response_body["lists"].keys()),
-            ["foo-list"],
-            response_body["lists"].keys(),
-        )
-
-        # Make sure the list includes the room we are joined to
-        self.assertListEqual(
-            list(response_body["lists"]["foo-list"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [room_id],
-                }
-            ],
-            response_body["lists"]["foo-list"],
-        )
-
     def test_wait_for_sync_token(self) -> None:
         """
         Test that worker will wait until it catches up to the given token
@@ -622,57 +581,6 @@ class SlidingSyncTestCase(SlidingSyncBase):
             response_body["lists"].keys(),
         )
 
-        # Make sure the lists have the correct rooms
-        self.assertListEqual(
-            list(response_body["lists"]["all"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [
-                        invite_room_id,
-                        room_id,
-                        invited_dm_room_id,
-                        joined_dm_room_id,
-                    ],
-                }
-            ],
-            list(response_body["lists"]["all"]),
-        )
-        self.assertListEqual(
-            list(response_body["lists"]["dms"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [invited_dm_room_id, joined_dm_room_id],
-                }
-            ],
-            list(response_body["lists"]["dms"]),
-        )
-        self.assertListEqual(
-            list(response_body["lists"]["non-dms"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [invite_room_id, room_id],
-                }
-            ],
-            list(response_body["lists"]["non-dms"]),
-        )
-        self.assertListEqual(
-            list(response_body["lists"]["room-invites"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [invite_room_id],
-                }
-            ],
-            list(response_body["lists"]["room-invites"]),
-        )
-
         # Ensure DM's are correctly marked
         self.assertDictEqual(
             {
@@ -756,28 +664,6 @@ class SlidingSyncTestCase(SlidingSyncBase):
             channel.json_body["lists"].keys(),
         )
 
-        # Make sure the lists have the correct rooms
-        self.assertListEqual(
-            list(channel.json_body["lists"]["all-list"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [space_room_id, room_id],
-                }
-            ],
-        )
-        self.assertListEqual(
-            list(channel.json_body["lists"]["foo-list"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [space_room_id],
-                }
-            ],
-        )
-
         # Everyone leaves the encrypted space room
         self.helper.leave(space_room_id, user1_id, tok=user1_tok)
         self.helper.leave(space_room_id, user2_id, tok=user2_tok)
@@ -809,28 +695,6 @@ class SlidingSyncTestCase(SlidingSyncBase):
         )
         self.assertEqual(channel.code, 200, channel.json_body)
 
-        # Make sure the lists have the correct rooms even though we `newly_left`
-        self.assertListEqual(
-            list(channel.json_body["lists"]["all-list"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [space_room_id, room_id],
-                }
-            ],
-        )
-        self.assertListEqual(
-            list(channel.json_body["lists"]["foo-list"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [space_room_id],
-                }
-            ],
-        )
-
     def test_sort_list(self) -> None:
         """
         Test that the `lists` are sorted by `stream_ordering`
@@ -866,19 +730,6 @@ class SlidingSyncTestCase(SlidingSyncBase):
             response_body["lists"].keys(),
         )
 
-        # Make sure the list is sorted in the way we expect
-        self.assertListEqual(
-            list(response_body["lists"]["foo-list"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 99],
-                    "room_ids": [room_id2, room_id1, room_id3],
-                }
-            ],
-            response_body["lists"]["foo-list"],
-        )
-
     def test_sliced_windows(self) -> None:
         """
         Test that the `lists` `ranges` are sliced correctly. Both sides of each range
@@ -909,17 +760,11 @@ class SlidingSyncTestCase(SlidingSyncBase):
             ["foo-list"],
             response_body["lists"].keys(),
         )
-        # Make sure the list is sorted in the way we expect
-        self.assertListEqual(
-            list(response_body["lists"]["foo-list"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 0],
-                    "room_ids": [room_id3],
-                }
-            ],
-            response_body["lists"]["foo-list"],
+        # Make sure the rooms are as we expect
+        self.assertIncludes(
+            set(response_body["rooms"].keys()),
+            {room_id3},
+            exact=True,
         )
 
         # Make the Sliding Sync request for the first two rooms
@@ -940,17 +785,11 @@ class SlidingSyncTestCase(SlidingSyncBase):
             ["foo-list"],
             response_body["lists"].keys(),
         )
-        # Make sure the list is sorted in the way we expect
-        self.assertListEqual(
-            list(response_body["lists"]["foo-list"]["ops"]),
-            [
-                {
-                    "op": "SYNC",
-                    "range": [0, 1],
-                    "room_ids": [room_id3, room_id2],
-                }
-            ],
-            response_body["lists"]["foo-list"],
+        # Make sure the rooms are as we expect
+        self.assertIncludes(
+            set(response_body["rooms"].keys()),
+            {room_id3, room_id2},
+            exact=True,
         )
 
     def test_rooms_with_no_updates_do_not_come_down_incremental_sync(self) -> None: