From 8624d45225c52bf14d078c76a3f315a092cb6c48 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 5 Sep 2024 14:41:03 +0100 Subject: [PATCH 1/4] Remove list ops from the sliding sync response --- synapse/rest/client/sync.py | 18 -- .../client/sliding_sync/test_rooms_meta.py | 15 -- .../sliding_sync/test_rooms_required_state.py | 30 ++- .../client/sliding_sync/test_sliding_sync.py | 181 +----------------- 4 files changed, 24 insertions(+), 220 deletions(-) diff --git a/synapse/rest/client/sync.py b/synapse/rest/client/sync.py index cc9fbfe546..fde4af3c85 100644 --- a/synapse/rest/client/sync.py +++ b/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 @@ async def encode_response( 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 diff --git a/tests/rest/client/sliding_sync/test_rooms_meta.py b/tests/rest/client/sliding_sync/test_rooms_meta.py index 4ed49040c1..0e8923f9e9 100644 --- a/tests/rest/client/sliding_sync/test_rooms_meta.py +++ b/tests/rest/client/sliding_sync/test_rooms_meta.py @@ -587,21 +587,6 @@ def test_rooms_bump_stamp(self) -> None: response_body["lists"].keys(), ) - # Make sure the list includes the rooms in the right order - self.assertListEqual( - list(response_body["lists"]["foo-list"]["ops"]), - [ - { - "op": "SYNC", - "range": [0, 1], - # room1 sorts before room2 because it has the latest event (the - # reaction) - "room_ids": [room_id1, room_id2], - } - ], - response_body["lists"]["foo-list"], - ) - # 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( 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 91ac6c5a0e..91eae06522 100644 --- a/tests/rest/client/sliding_sync/test_rooms_required_state.py +++ b/tests/rest/client/sliding_sync/test_rooms_required_state.py @@ -735,14 +735,13 @@ def test_rooms_required_state_partial_state(self) -> None: 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 @@ def test_rooms_required_state_partial_state(self) -> None: # 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 diff --git a/tests/rest/client/sliding_sync/test_sliding_sync.py b/tests/rest/client/sliding_sync/test_sliding_sync.py index 930cb5ef45..5d16b91e93 100644 --- a/tests/rest/client/sliding_sync/test_sliding_sync.py +++ b/tests/rest/client/sliding_sync/test_sliding_sync.py @@ -348,47 +348,6 @@ def _create_dm_room( 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 @@ def test_filter_list(self) -> None: 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 @@ def test_filter_regardless_of_membership_server_left_room(self) -> None: 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 @@ def test_filter_regardless_of_membership_server_left_room(self) -> None: ) 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 @@ def test_sort_list(self) -> None: 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 @@ def test_sliced_windows(self) -> None: ["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 @@ def test_sliced_windows(self) -> None: ["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: From e7487612be836bcc373715317817aea68bf07681 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 5 Sep 2024 14:50:13 +0100 Subject: [PATCH 2/4] Remove the sync ops from the code --- synapse/handlers/sliding_sync/extensions.py | 28 ++++-------- synapse/handlers/sliding_sync/room_lists.py | 50 ++++++++------------- synapse/types/handlers/sliding_sync.py | 19 -------- 3 files changed, 27 insertions(+), 70 deletions(-) diff --git a/synapse/handlers/sliding_sync/extensions.py b/synapse/handlers/sliding_sync/extensions.py index 6f37cc3462..6cfa568710 100644 --- a/synapse/handlers/sliding_sync/extensions.py +++ b/synapse/handlers/sliding_sync/extensions.py @@ -20,6 +20,7 @@ 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.handlers.sliding_sync import ( HaveSentRoomFlag, MutablePerConnectionState, - OperationType, PerConnectionState, SlidingSyncConfig, SlidingSyncResult, @@ -60,7 +60,7 @@ async def get_extensions_response( 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 @@ def find_relevant_room_ids_for_extension( 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 @@ def find_relevant_room_ids_for_extension( 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 @@ async def get_e2ee_extension_response( 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 @@ async def get_receipts_extension_response( 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 @@ async def get_receipts_extension_response( 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, diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 1423d6ca53..b9d8a26446 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -61,7 +61,6 @@ ) from synapse.types.handlers.sliding_sync import ( HaveSentRoomFlag, - OperationType, PerConnectionState, RoomSyncConfig, SlidingSyncConfig, @@ -93,13 +92,22 @@ class SlidingSyncInterestedRooms: extensions may have updates in these 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] room_membership_for_user_map: Mapping[str, "_RoomMembershipForUser"] +@attr.s(auto_attribs=True, slots=True, frozen=True) +class SlidingSyncListResult(SlidingSyncResult.SlidingWindowList): + """Extension to the lists response that allows us to track the matching + room IDs in the list. + """ + + 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. @@ -232,7 +240,7 @@ async def _compute_interested_rooms_new_tables( 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 @@ -379,11 +387,9 @@ async def _compute_interested_rooms_new_tables( 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. @@ -412,17 +418,8 @@ async def _compute_interested_rooms_new_tables( 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: @@ -513,7 +510,7 @@ async def _compute_interested_rooms_fallback( ) # 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 @@ -566,11 +563,9 @@ async def _compute_interested_rooms_fallback( 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. @@ -599,17 +594,8 @@ async def _compute_interested_rooms_fallback( 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: diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index 84a88bf784..79157364c7 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -29,7 +29,6 @@ Optional, Sequence, Set, - Tuple, TypeVar, cast, ) @@ -218,27 +217,9 @@ class SlidingWindowList: 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: From b715c8b2923e58b5bb934d852e4a1282710f8efb Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Thu, 5 Sep 2024 14:52:50 +0100 Subject: [PATCH 3/4] Newsfile --- changelog.d/17671.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17671.misc diff --git a/changelog.d/17671.misc b/changelog.d/17671.misc new file mode 100644 index 0000000000..a0c4eb7222 --- /dev/null +++ b/changelog.d/17671.misc @@ -0,0 +1 @@ +Remove unspecced list operations from the sliding sync response. From b4a1784d66a171a001c1d159e269f2d8dc025bc2 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 10 Sep 2024 10:16:51 +0100 Subject: [PATCH 4/4] Update doc --- synapse/handlers/sliding_sync/room_lists.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index b9d8a26446..14242de1c7 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -101,8 +101,10 @@ class SlidingSyncInterestedRooms: @attr.s(auto_attribs=True, slots=True, frozen=True) class SlidingSyncListResult(SlidingSyncResult.SlidingWindowList): - """Extension to the lists response that allows us to track the matching - room IDs in the list. + """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