diff --git a/changelog.d/17707.feature b/changelog.d/17707.feature new file mode 100644 index 0000000000..98a00ca34b --- /dev/null +++ b/changelog.d/17707.feature @@ -0,0 +1 @@ +Return room tags in Sliding Sync account data extension. diff --git a/synapse/handlers/sliding_sync/extensions.py b/synapse/handlers/sliding_sync/extensions.py index 7c2f8a2569..287f4b04ad 100644 --- a/synapse/handlers/sliding_sync/extensions.py +++ b/synapse/handlers/sliding_sync/extensions.py @@ -14,7 +14,19 @@ import itertools import logging -from typing import TYPE_CHECKING, AbstractSet, Dict, Mapping, Optional, Sequence, Set +from typing import ( + TYPE_CHECKING, + AbstractSet, + ChainMap, + Dict, + List, + Mapping, + MutableMapping, + Optional, + Sequence, + Set, + cast, +) from typing_extensions import assert_never @@ -381,29 +393,47 @@ async def get_account_data_extension_response( ) ) + # TODO: This should take into account the `from_token` and `to_token` have_push_rules_changed = await self.store.have_push_rules_changed_for_user( user_id, from_token.stream_token.push_rules_key ) if have_push_rules_changed: - global_account_data_map = dict(global_account_data_map) # TODO: This should take into account the `from_token` and `to_token` global_account_data_map[ AccountDataTypes.PUSH_RULES ] = await self.push_rules_handler.push_rules_for_user(sync_config.user) else: # TODO: This should take into account the `to_token` - all_global_account_data = await self.store.get_global_account_data_for_user( - user_id + immutable_global_account_data_map = ( + await self.store.get_global_account_data_for_user(user_id) ) - global_account_data_map = dict(all_global_account_data) - # TODO: This should take into account the `to_token` - global_account_data_map[ - AccountDataTypes.PUSH_RULES - ] = await self.push_rules_handler.push_rules_for_user(sync_config.user) + # Use a `ChainMap` to avoid copying the immutable data from the cache + global_account_data_map = ChainMap( + { + # TODO: This should take into account the `to_token` + AccountDataTypes.PUSH_RULES: await self.push_rules_handler.push_rules_for_user( + sync_config.user + ) + }, + # Cast is safe because `ChainMap` only mutates the top-most map, + # see https://github.com/python/typeshed/issues/8430 + cast( + MutableMapping[str, JsonMapping], immutable_global_account_data_map + ), + ) # Fetch room account data - account_data_by_room_map: Mapping[str, Mapping[str, JsonMapping]] = {} + # + # List of -> Mapping from room_id to mapping of `type` to `content` of room + # account data events. + # + # This is is a list so we can avoid making copies of immutable data and instead + # just provide multiple maps that need to be combined. Normally, we could + # reach for `ChainMap` in this scenario, but this is a nested map and accessing + # the ChainMap by room_id won't combine the two maps for that room (we would + # need a new `NestedChainMap` type class). + account_data_by_room_maps: List[Mapping[str, Mapping[str, JsonMapping]]] = [] relevant_room_ids = self.find_relevant_room_ids_for_extension( requested_lists=account_data_request.lists, requested_room_ids=account_data_request.rooms, @@ -418,22 +448,66 @@ async def get_account_data_extension_response( user_id, from_token.stream_token.account_data_key ) ) + + # Add room tags + # + # TODO: This should take into account the `from_token` and `to_token` + tags_by_room = await self.store.get_updated_tags( + user_id, from_token.stream_token.account_data_key + ) + for room_id, tags in tags_by_room.items(): + account_data_by_room_map.setdefault(room_id, {})[ + AccountDataTypes.TAG + ] = {"tags": tags} + + account_data_by_room_maps.append(account_data_by_room_map) else: # TODO: This should take into account the `to_token` - account_data_by_room_map = ( + immutable_account_data_by_room_map = ( await self.store.get_room_account_data_for_user(user_id) ) + account_data_by_room_maps.append(immutable_account_data_by_room_map) - # Filter down to the relevant rooms - account_data_by_room_map = { - room_id: account_data_map - for room_id, account_data_map in account_data_by_room_map.items() - if room_id in relevant_room_ids - } + # Add room tags + # + # TODO: This should take into account the `to_token` + tags_by_room = await self.store.get_tags_for_user(user_id) + account_data_by_room_maps.append( + { + room_id: {AccountDataTypes.TAG: {"tags": tags}} + for room_id, tags in tags_by_room.items() + } + ) + + # Filter down to the relevant rooms ... and combine the maps + relevant_account_data_by_room_map: MutableMapping[ + str, Mapping[str, JsonMapping] + ] = {} + for room_id in relevant_room_ids: + # We want to avoid adding empty maps for relevant rooms that have no room + # account data so do a quick check to see if it's in any of the maps. + is_room_in_maps = False + for room_map in account_data_by_room_maps: + if room_id in room_map: + is_room_in_maps = True + break + + # If we found the room in any of the maps, combine the maps for that room + if is_room_in_maps: + relevant_account_data_by_room_map[room_id] = ChainMap( + {}, + *( + # Cast is safe because `ChainMap` only mutates the top-most map, + # see https://github.com/python/typeshed/issues/8430 + cast(MutableMapping[str, JsonMapping], room_map[room_id]) + for room_map in account_data_by_room_maps + if room_map.get(room_id) + ), + ) return SlidingSyncResult.Extensions.AccountDataExtension( global_account_data_map=global_account_data_map, - account_data_by_room_map=account_data_by_room_map, + account_data_by_room_map=relevant_account_data_by_room_map, ) @trace diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index 966393869b..b30639b4e6 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -177,7 +177,7 @@ async def get_room_account_data_for_user( def get_room_account_data_for_user_txn( txn: LoggingTransaction, - ) -> Dict[str, Dict[str, JsonDict]]: + ) -> Dict[str, Dict[str, JsonMapping]]: # The 'content != '{}' condition below prevents us from using # `simple_select_list_txn` here, as it doesn't support conditions # other than 'equals'. @@ -194,7 +194,7 @@ def get_room_account_data_for_user_txn( txn.execute(sql, (user_id,)) - by_room: Dict[str, Dict[str, JsonDict]] = {} + by_room: Dict[str, Dict[str, JsonMapping]] = {} for room_id, account_data_type, content in txn: room_data = by_room.setdefault(room_id, {}) @@ -394,7 +394,7 @@ def get_updated_room_account_data_txn( async def get_updated_global_account_data_for_user( self, user_id: str, stream_id: int - ) -> Mapping[str, JsonMapping]: + ) -> Dict[str, JsonMapping]: """Get all the global account_data that's changed for a user. Args: @@ -407,7 +407,7 @@ async def get_updated_global_account_data_for_user( def get_updated_global_account_data_for_user( txn: LoggingTransaction, - ) -> Dict[str, JsonDict]: + ) -> Dict[str, JsonMapping]: sql = """ SELECT account_data_type, content FROM account_data WHERE user_id = ? AND stream_id > ? @@ -429,7 +429,7 @@ def get_updated_global_account_data_for_user( async def get_updated_room_account_data_for_user( self, user_id: str, stream_id: int - ) -> Dict[str, Dict[str, JsonDict]]: + ) -> Dict[str, Dict[str, JsonMapping]]: """Get all the room account_data that's changed for a user. Args: @@ -442,14 +442,14 @@ async def get_updated_room_account_data_for_user( def get_updated_room_account_data_for_user_txn( txn: LoggingTransaction, - ) -> Dict[str, Dict[str, JsonDict]]: + ) -> Dict[str, Dict[str, JsonMapping]]: sql = """ SELECT room_id, account_data_type, content FROM room_account_data WHERE user_id = ? AND stream_id > ? """ txn.execute(sql, (user_id, stream_id)) - account_data_by_room: Dict[str, Dict[str, JsonDict]] = {} + account_data_by_room: Dict[str, Dict[str, JsonMapping]] = {} for row in txn: room_account_data = account_data_by_room.setdefault(row[0], {}) room_account_data[row[1]] = db_to_json(row[2]) diff --git a/synapse/types/handlers/sliding_sync.py b/synapse/types/handlers/sliding_sync.py index 48badeacda..149920f883 100644 --- a/synapse/types/handlers/sliding_sync.py +++ b/synapse/types/handlers/sliding_sync.py @@ -314,8 +314,8 @@ class AccountDataExtension: """The Account Data extension (MSC3959) Attributes: - global_account_data_map: Mapping from `type` to `content` of global account - data events. + global_account_data_map: Mapping from `type` to `content` of global + account data events. account_data_by_room_map: Mapping from room_id to mapping of `type` to `content` of room account data events. """ diff --git a/tests/rest/client/sliding_sync/test_extension_account_data.py b/tests/rest/client/sliding_sync/test_extension_account_data.py index 6cc883a4be..03b2db39b9 100644 --- a/tests/rest/client/sliding_sync/test_extension_account_data.py +++ b/tests/rest/client/sliding_sync/test_extension_account_data.py @@ -80,18 +80,23 @@ def test_no_data_initial_sync(self) -> None: } response_body, _ = self.do_sync(sync_body, tok=user1_tok) + global_account_data_map = { + global_event["type"]: global_event["content"] + for global_event in response_body["extensions"]["account_data"].get( + "global" + ) + } self.assertIncludes( - { - global_event["type"] - for global_event in response_body["extensions"]["account_data"].get( - "global" - ) - }, + global_account_data_map.keys(), # Even though we don't have any global account data set, Synapse saves some # default push rules for us. {AccountDataTypes.PUSH_RULES}, exact=True, ) + # Push rules are a giant chunk of JSON data so we will just assume the value is correct if they key is here. + # global_account_data_map[AccountDataTypes.PUSH_RULES] + + # No room account data for this test self.assertIncludes( response_body["extensions"]["account_data"].get("rooms").keys(), set(), @@ -121,16 +126,19 @@ def test_no_data_incremental_sync(self) -> None: # There has been no account data changes since the `from_token` so we shouldn't # see any account data here. + global_account_data_map = { + global_event["type"]: global_event["content"] + for global_event in response_body["extensions"]["account_data"].get( + "global" + ) + } self.assertIncludes( - { - global_event["type"] - for global_event in response_body["extensions"]["account_data"].get( - "global" - ) - }, + global_account_data_map.keys(), set(), exact=True, ) + + # No room account data for this test self.assertIncludes( response_body["extensions"]["account_data"].get("rooms").keys(), set(), @@ -165,16 +173,24 @@ def test_global_account_data_initial_sync(self) -> None: response_body, _ = self.do_sync(sync_body, tok=user1_tok) # It should show us all of the global account data + global_account_data_map = { + global_event["type"]: global_event["content"] + for global_event in response_body["extensions"]["account_data"].get( + "global" + ) + } self.assertIncludes( - { - global_event["type"] - for global_event in response_body["extensions"]["account_data"].get( - "global" - ) - }, + global_account_data_map.keys(), {AccountDataTypes.PUSH_RULES, "org.matrix.foobarbaz"}, exact=True, ) + # Push rules are a giant chunk of JSON data so we will just assume the value is correct if they key is here. + # global_account_data_map[AccountDataTypes.PUSH_RULES] + self.assertEqual( + global_account_data_map["org.matrix.foobarbaz"], {"foo": "bar"} + ) + + # No room account data for this test self.assertIncludes( response_body["extensions"]["account_data"].get("rooms").keys(), set(), @@ -220,17 +236,23 @@ def test_global_account_data_incremental_sync(self) -> None: # Make an incremental Sliding Sync request with the account_data extension enabled response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) + global_account_data_map = { + global_event["type"]: global_event["content"] + for global_event in response_body["extensions"]["account_data"].get( + "global" + ) + } self.assertIncludes( - { - global_event["type"] - for global_event in response_body["extensions"]["account_data"].get( - "global" - ) - }, + global_account_data_map.keys(), # We should only see the new global account data that happened after the `from_token` {"org.matrix.doodardaz"}, exact=True, ) + self.assertEqual( + global_account_data_map["org.matrix.doodardaz"], {"doo": "dar"} + ) + + # No room account data for this test self.assertIncludes( response_body["extensions"]["account_data"].get("rooms").keys(), set(), @@ -255,6 +277,15 @@ def test_room_account_data_initial_sync(self) -> None: content={"roo": "rar"}, ) ) + # Add a room tag to mark the room as a favourite + self.get_success( + self.account_data_handler.add_tag_to_room( + user_id=user1_id, + room_id=room_id1, + tag="m.favourite", + content={}, + ) + ) # Create another room with some room account data room_id2 = self.helper.create_room_as(user1_id, tok=user1_tok) @@ -266,6 +297,15 @@ def test_room_account_data_initial_sync(self) -> None: content={"roo": "rar"}, ) ) + # Add a room tag to mark the room as a favourite + self.get_success( + self.account_data_handler.add_tag_to_room( + user_id=user1_id, + room_id=room_id2, + tag="m.favourite", + content={}, + ) + ) # Make an initial Sliding Sync request with the account_data extension enabled sync_body = { @@ -294,16 +334,21 @@ def test_room_account_data_initial_sync(self) -> None: {room_id1}, exact=True, ) + account_data_map = { + event["type"]: event["content"] + for event in response_body["extensions"]["account_data"] + .get("rooms") + .get(room_id1) + } self.assertIncludes( - { - event["type"] - for event in response_body["extensions"]["account_data"] - .get("rooms") - .get(room_id1) - }, - {"org.matrix.roorarraz"}, + account_data_map.keys(), + {"org.matrix.roorarraz", AccountDataTypes.TAG}, exact=True, ) + self.assertEqual(account_data_map["org.matrix.roorarraz"], {"roo": "rar"}) + self.assertEqual( + account_data_map[AccountDataTypes.TAG], {"tags": {"m.favourite": {}}} + ) def test_room_account_data_incremental_sync(self) -> None: """ @@ -323,6 +368,15 @@ def test_room_account_data_incremental_sync(self) -> None: content={"roo": "rar"}, ) ) + # Add a room tag to mark the room as a favourite + self.get_success( + self.account_data_handler.add_tag_to_room( + user_id=user1_id, + room_id=room_id1, + tag="m.favourite", + content={}, + ) + ) # Create another room with some room account data room_id2 = self.helper.create_room_as(user1_id, tok=user1_tok) @@ -334,6 +388,15 @@ def test_room_account_data_incremental_sync(self) -> None: content={"roo": "rar"}, ) ) + # Add a room tag to mark the room as a favourite + self.get_success( + self.account_data_handler.add_tag_to_room( + user_id=user1_id, + room_id=room_id2, + tag="m.favourite", + content={}, + ) + ) sync_body = { "lists": {}, @@ -369,6 +432,23 @@ def test_room_account_data_incremental_sync(self) -> None: content={"roo": "rar"}, ) ) + # Add another room tag + self.get_success( + self.account_data_handler.add_tag_to_room( + user_id=user1_id, + room_id=room_id1, + tag="m.server_notice", + content={}, + ) + ) + self.get_success( + self.account_data_handler.add_tag_to_room( + user_id=user1_id, + room_id=room_id2, + tag="m.server_notice", + content={}, + ) + ) # Make an incremental Sliding Sync request with the account_data extension enabled response_body, _ = self.do_sync(sync_body, since=from_token, tok=user1_tok) @@ -383,16 +463,22 @@ def test_room_account_data_incremental_sync(self) -> None: exact=True, ) # We should only see the new room account data that happened after the `from_token` + account_data_map = { + event["type"]: event["content"] + for event in response_body["extensions"]["account_data"] + .get("rooms") + .get(room_id1) + } self.assertIncludes( - { - event["type"] - for event in response_body["extensions"]["account_data"] - .get("rooms") - .get(room_id1) - }, - {"org.matrix.roorarraz2"}, + account_data_map.keys(), + {"org.matrix.roorarraz2", AccountDataTypes.TAG}, exact=True, ) + self.assertEqual(account_data_map["org.matrix.roorarraz2"], {"roo": "rar"}) + self.assertEqual( + account_data_map[AccountDataTypes.TAG], + {"tags": {"m.favourite": {}, "m.server_notice": {}}}, + ) def test_wait_for_new_data(self) -> None: """