From 78055b2d37fa877fd11183c4a5d438728f1c320b Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 4 Sep 2024 15:55:47 +0100 Subject: [PATCH 01/11] Support filtering by 'tags' / 'not_tags' in SSS --- synapse/handlers/sliding_sync/room_lists.py | 23 +++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 1423d6ca53a..34e49008c10 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -50,6 +50,7 @@ from synapse.storage.databases.main.stream import CurrentStateDeltaMembership from synapse.storage.roommember import RoomsForUser, RoomsForUserSlidingSync from synapse.types import ( + JsonMapping, MutableStateMap, PersistedEventPosition, RoomStreamToken, @@ -301,6 +302,9 @@ async def _compute_interested_rooms_new_tables( ) dm_room_ids = await self._get_dm_rooms_for_user(user_id) + # Fetch the user tags for their rooms + room_tags = await self.store.get_tags_for_user(user_id) + # Handle state resets in the from -> to token range. state_reset_rooms = ( newly_left_room_map.keys() - room_membership_for_user_map.keys() @@ -349,6 +353,7 @@ async def _compute_interested_rooms_new_tables( list_config.filters, to_token, dm_room_ids, + room_tags, ) # Find which rooms are partially stated and may need to be filtered out @@ -1699,6 +1704,7 @@ async def filter_rooms_using_tables( filters: SlidingSyncConfig.SlidingSyncList.Filters, to_token: StreamToken, dm_room_ids: AbstractSet[str], + room_tags: Mapping[str, Mapping[str, JsonMapping]], ) -> Dict[str, RoomsForUserSlidingSync]: """ Filter rooms based on the sync request. @@ -1710,6 +1716,7 @@ async def filter_rooms_using_tables( filters: Filters to apply to_token: We filter based on the state of the room at this token dm_room_ids: Set of room IDs which are DMs + room_tags: Mapping of room ID to tags Returns: A filtered dictionary of room IDs along with membership information in the @@ -1795,9 +1802,21 @@ async def filter_rooms_using_tables( # ) raise NotImplementedError() - if filters.tags is not None or filters.not_tags is not None: + if filters.tags is not None: with start_active_span("filters.tags"): - raise NotImplementedError() + filtered_room_id_set = { + room_id + for room_id in filtered_room_id_set + if set(room_tags.get(room_id, [])) & set(filters.tags) + } + + if filters.not_tags is not None: + with start_active_span("filters.not_tags"): + filtered_room_id_set = { + room_id + for room_id in filtered_room_id_set + if not set(room_tags.get(room_id, [])) & set(filters.not_tags) + } # Assemble a new sync room map but only with the `filtered_room_id_set` return {room_id: sync_room_map[room_id] for room_id in filtered_room_id_set} From 31257abf242fa93507f581ae9979c6fb92899ef6 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 4 Sep 2024 15:59:33 +0100 Subject: [PATCH 02/11] changelog --- changelog.d/17662.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17662.feature diff --git a/changelog.d/17662.feature b/changelog.d/17662.feature new file mode 100644 index 00000000000..46d6037a180 --- /dev/null +++ b/changelog.d/17662.feature @@ -0,0 +1 @@ +Add support for the `tags` and `not_tags` filters for simplified sliding sync. From 7e9a9cfbed62b93dc9f75bf1a222bccc47213b86 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 4 Sep 2024 18:16:38 +0100 Subject: [PATCH 03/11] Add test --- tests/handlers/test_sliding_sync.py | 119 ++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 2ef9f665f9f..3b36eb68a77 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -3006,6 +3006,7 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.sliding_sync_handler = self.hs.get_sliding_sync_handler() self.store = self.hs.get_datastores().main self.event_sources = hs.get_event_sources() + self.account_data_handler = hs.get_account_data_handler() def _get_sync_room_ids_for_user( self, @@ -3209,6 +3210,124 @@ def test_filter_dm_rooms(self) -> None: self.assertEqual(falsy_filtered_room_map.keys(), {room_id}) + def test_filter_rooms_by_tags(self) -> None: + """ + Test `filter.tags` for rooms with given tags + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + # Create a room with no tags + self.helper.create_room_as(user1_id, tok=user1_tok) + + footag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + bartag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + + tag_data = { + footag_room_id: {"foo": {}}, + bartag_room_id: {"bar": {}}, + } + + after_rooms_token = self.event_sources.get_current_token() + + # Get the rooms the user should be syncing with + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, + ) + + # Try with `tags=foo` + foo_filtered_room_map = self.get_success( + self.sliding_sync_handler.room_lists.filter_rooms_using_tables( + UserID.from_string(user1_id), + sync_room_map, + SlidingSyncConfig.SlidingSyncList.Filters( + tags={"foo"}, + ), + after_rooms_token, + {}, + tag_data, + ) + ) + + self.assertEqual(foo_filtered_room_map.keys(), {footag_room_id}) + + # Try with a random tag we didn't add + foobar_filtered_room_map = self.get_success( + self.sliding_sync_handler.room_lists.filter_rooms_using_tables( + UserID.from_string(user1_id), + sync_room_map, + SlidingSyncConfig.SlidingSyncList.Filters( + tags={"flomp"}, + ), + after_rooms_token, + {}, + tag_data, + ) + ) + + self.assertEqual(len(foobar_filtered_room_map), 0) + + def test_filter_rooms_by_not_tags(self) -> None: + """ + Test `filter.not_tags` for excluding rooms with given tags + """ + user1_id = self.register_user("user1", "pass") + user1_tok = self.login(user1_id, "pass") + + # Create a room with no tags + untagged_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + + footag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + bartag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + + tag_data = { + footag_room_id: {"foo": {}}, + bartag_room_id: {"bar": {}}, + } + + after_rooms_token = self.event_sources.get_current_token() + + # Get the rooms the user should be syncing with + sync_room_map = self._get_sync_room_ids_for_user( + UserID.from_string(user1_id), + from_token=None, + to_token=after_rooms_token, + ) + + # Try with `not_tags=foo` + foo_filtered_room_map = self.get_success( + self.sliding_sync_handler.room_lists.filter_rooms_using_tables( + UserID.from_string(user1_id), + sync_room_map, + SlidingSyncConfig.SlidingSyncList.Filters( + not_tags={"foo"}, + ), + after_rooms_token, + {}, + tag_data, + ) + ) + + self.assertEqual(foo_filtered_room_map.keys(), {untagged_room_id, bartag_room_id}) + + # Try with not_tags=[foo,bar] + foobar_filtered_room_map = self.get_success( + self.sliding_sync_handler.room_lists.filter_rooms_using_tables( + UserID.from_string(user1_id), + sync_room_map, + SlidingSyncConfig.SlidingSyncList.Filters( + not_tags={"foo", "bar"}, + ), + after_rooms_token, + {}, + tag_data, + ) + ) + + self.assertEqual(foobar_filtered_room_map.keys(), {untagged_room_id}) + def test_filter_encrypted_rooms(self) -> None: """ Test `filter.is_encrypted` for encrypted rooms From 66350fc7f10d8724fe516ed63d53722b77d54830 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 4 Sep 2024 18:19:00 +0100 Subject: [PATCH 04/11] Format --- tests/handlers/test_sliding_sync.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 3b36eb68a77..30d81c3f397 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -3310,7 +3310,9 @@ def test_filter_rooms_by_not_tags(self) -> None: ) ) - self.assertEqual(foo_filtered_room_map.keys(), {untagged_room_id, bartag_room_id}) + self.assertEqual( + foo_filtered_room_map.keys(), {untagged_room_id, bartag_room_id} + ) # Try with not_tags=[foo,bar] foobar_filtered_room_map = self.get_success( From 5caf7240687e399a72c90a09aa9cda58cf232b62 Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 4 Sep 2024 18:51:31 +0100 Subject: [PATCH 05/11] Hopefully make mypy happy --- tests/handlers/test_sliding_sync.py | 62 ++++++++++++++++------------- 1 file changed, 34 insertions(+), 28 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index 30d81c3f397..ef210b097ff 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -18,7 +18,7 @@ # # import logging -from typing import Dict, List, Optional +from typing import Dict, List, Mapping, Optional from unittest.mock import patch from parameterized import parameterized @@ -45,7 +45,7 @@ from synapse.rest.client import knock, login, room from synapse.server import HomeServer from synapse.storage.util.id_generators import MultiWriterIdGenerator -from synapse.types import JsonDict, StreamToken, UserID +from synapse.types import JsonDict, JsonMapping, StreamToken, UserID from synapse.types.handlers.sliding_sync import SlidingSyncConfig from synapse.util import Clock @@ -3223,7 +3223,7 @@ def test_filter_rooms_by_tags(self) -> None: footag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) bartag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) - tag_data = { + tag_data: Mapping[str, Mapping[str, JsonMapping]] = { footag_room_id: {"foo": {}}, bartag_room_id: {"bar": {}}, } @@ -3231,22 +3231,22 @@ def test_filter_rooms_by_tags(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self._get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, + room_membership_for_user_map = self.get_success( + self.store.get_sliding_sync_rooms_for_user( + user1_id + ) ) # Try with `tags=foo` foo_filtered_room_map = self.get_success( self.sliding_sync_handler.room_lists.filter_rooms_using_tables( - UserID.from_string(user1_id), - sync_room_map, + user1_id, + room_membership_for_user_map, SlidingSyncConfig.SlidingSyncList.Filters( - tags={"foo"}, + tags=["foo"], ), after_rooms_token, - {}, + set(), tag_data, ) ) @@ -3256,13 +3256,13 @@ def test_filter_rooms_by_tags(self) -> None: # Try with a random tag we didn't add foobar_filtered_room_map = self.get_success( self.sliding_sync_handler.room_lists.filter_rooms_using_tables( - UserID.from_string(user1_id), - sync_room_map, + user1_id, + room_membership_for_user_map, SlidingSyncConfig.SlidingSyncList.Filters( - tags={"flomp"}, + tags=["flomp"], ), after_rooms_token, - {}, + set(), tag_data, ) ) @@ -3282,7 +3282,7 @@ def test_filter_rooms_by_not_tags(self) -> None: footag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) bartag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) - tag_data = { + tag_data: Mapping[str, Mapping[str, JsonMapping]] = { footag_room_id: {"foo": {}}, bartag_room_id: {"bar": {}}, } @@ -3290,22 +3290,28 @@ def test_filter_rooms_by_not_tags(self) -> None: after_rooms_token = self.event_sources.get_current_token() # Get the rooms the user should be syncing with - sync_room_map = self._get_sync_room_ids_for_user( - UserID.from_string(user1_id), - from_token=None, - to_token=after_rooms_token, + # sync_room_map = self._get_sync_room_ids_for_user( + # UserID.from_string(user1_id), + # from_token=None, + # to_token=after_rooms_token, + # ) + + room_membership_for_user_map = self.get_success( + self.store.get_sliding_sync_rooms_for_user( + user1_id + ) ) # Try with `not_tags=foo` foo_filtered_room_map = self.get_success( self.sliding_sync_handler.room_lists.filter_rooms_using_tables( - UserID.from_string(user1_id), - sync_room_map, + user1_id, + room_membership_for_user_map, SlidingSyncConfig.SlidingSyncList.Filters( - not_tags={"foo"}, + not_tags=["foo"], ), after_rooms_token, - {}, + set(), tag_data, ) ) @@ -3317,13 +3323,13 @@ def test_filter_rooms_by_not_tags(self) -> None: # Try with not_tags=[foo,bar] foobar_filtered_room_map = self.get_success( self.sliding_sync_handler.room_lists.filter_rooms_using_tables( - UserID.from_string(user1_id), - sync_room_map, + user1_id, + room_membership_for_user_map, SlidingSyncConfig.SlidingSyncList.Filters( - not_tags={"foo", "bar"}, + not_tags=["foo", "bar"], ), after_rooms_token, - {}, + set(), tag_data, ) ) From 7761106faf84b8b6ffe923da00f2c625a73d77bf Mon Sep 17 00:00:00 2001 From: David Baker Date: Wed, 4 Sep 2024 18:54:03 +0100 Subject: [PATCH 06/11] Ruff --- tests/handlers/test_sliding_sync.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/tests/handlers/test_sliding_sync.py b/tests/handlers/test_sliding_sync.py index ef210b097ff..33a2440a6bc 100644 --- a/tests/handlers/test_sliding_sync.py +++ b/tests/handlers/test_sliding_sync.py @@ -3232,9 +3232,7 @@ def test_filter_rooms_by_tags(self) -> None: # Get the rooms the user should be syncing with room_membership_for_user_map = self.get_success( - self.store.get_sliding_sync_rooms_for_user( - user1_id - ) + self.store.get_sliding_sync_rooms_for_user(user1_id) ) # Try with `tags=foo` @@ -3297,9 +3295,7 @@ def test_filter_rooms_by_not_tags(self) -> None: # ) room_membership_for_user_map = self.get_success( - self.store.get_sliding_sync_rooms_for_user( - user1_id - ) + self.store.get_sliding_sync_rooms_for_user(user1_id) ) # Try with `not_tags=foo` From 6662fd5ba38865e231876ddedac17ec4efc49188 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 12 Sep 2024 16:00:07 -0500 Subject: [PATCH 07/11] Add more test cases --- .../client/sliding_sync/test_lists_filters.py | 153 ++++++++++++++++-- 1 file changed, 140 insertions(+), 13 deletions(-) diff --git a/tests/rest/client/sliding_sync/test_lists_filters.py b/tests/rest/client/sliding_sync/test_lists_filters.py index 27ab762cb45..4ca599a1918 100644 --- a/tests/rest/client/sliding_sync/test_lists_filters.py +++ b/tests/rest/client/sliding_sync/test_lists_filters.py @@ -1149,6 +1149,27 @@ def test_filters_room_types(self) -> None: exact=True, ) + # Just make sure we know what happens when you specify an empty list of room_types + # (we should find nothing) + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 99]], + "required_state": [], + "timeline_limit": 0, + "filters": { + "room_types": [], + }, + }, + } + } + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + self.assertIncludes( + set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), + set(), + exact=True, + ) + def test_filters_not_room_types(self) -> None: """ Test `filters.not_room_types` for different room types @@ -1284,6 +1305,27 @@ def test_filters_not_room_types(self) -> None: exact=True, ) + # Just make sure we know what happens when you specify an empty list of not_room_types + # (we should find all of the rooms) + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 99]], + "required_state": [], + "timeline_limit": 0, + "filters": { + "not_room_types": [], + }, + }, + } + } + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + self.assertIncludes( + set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), + {room_id, foo_room_id, space_room_id}, + exact=True, + ) + def test_filters_room_types_server_left_room(self) -> None: """ Test that we can apply a `filters.room_types` against a room that everyone has left. @@ -1691,13 +1733,13 @@ def test_filters_tags(self) -> None: # Create a room with no tags self.helper.create_room_as(user1_id, tok=user1_tok) - footag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) - bartag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + foo_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + bar_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) # Add the "foo" tag to the room channel = self.make_request( method="PUT", - path=f"/user/{user1_id}/rooms/{footag_room_id}/tags/foo", + path=f"/user/{user1_id}/rooms/{foo_room_id}/tags/foo", content={}, access_token=user1_tok, ) @@ -1705,13 +1747,13 @@ def test_filters_tags(self) -> None: # Add the "bar" tag to the room channel = self.make_request( method="PUT", - path=f"/user/{user1_id}/rooms/{bartag_room_id}/tags/bar", + path=f"/user/{user1_id}/rooms/{bar_room_id}/tags/bar", content={}, access_token=user1_tok, ) self.assertEqual(channel.code, 200, channel.json_body) - # Try with `tags=foo` + # Try finding rooms with the "foo" tag sync_body = { "lists": { "foo-list": { @@ -1727,7 +1769,27 @@ def test_filters_tags(self) -> None: response_body, _ = self.do_sync(sync_body, tok=user1_tok) self.assertIncludes( set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), - {footag_room_id}, + {foo_room_id}, + exact=True, + ) + + # Try finding rooms with either "foo" or "bar" tags + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 99]], + "required_state": [], + "timeline_limit": 0, + "filters": { + "tags": ["foo", "bar"], + }, + }, + } + } + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + self.assertIncludes( + set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), + {foo_room_id, bar_room_id}, exact=True, ) @@ -1752,6 +1814,27 @@ def test_filters_tags(self) -> None: exact=True, ) + # Just make sure we know what happens when you specify an empty list of tags + # (we should find nothing) + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 99]], + "required_state": [], + "timeline_limit": 0, + "filters": { + "tags": [], + }, + }, + } + } + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + self.assertIncludes( + set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), + set(), + exact=True, + ) + def test_filters_not_tags(self) -> None: """ Test `filters.not_tags` for excluding rooms with given tags @@ -1762,13 +1845,13 @@ def test_filters_not_tags(self) -> None: # Create a room with no tags untagged_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) - footag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) - bartag_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + foo_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + bar_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) # Add the "foo" tag to the room channel = self.make_request( method="PUT", - path=f"/user/{user1_id}/rooms/{footag_room_id}/tags/foo", + path=f"/user/{user1_id}/rooms/{foo_room_id}/tags/foo", content={}, access_token=user1_tok, ) @@ -1776,13 +1859,13 @@ def test_filters_not_tags(self) -> None: # Add the "bar" tag to the room channel = self.make_request( method="PUT", - path=f"/user/{user1_id}/rooms/{bartag_room_id}/tags/bar", + path=f"/user/{user1_id}/rooms/{bar_room_id}/tags/bar", content={}, access_token=user1_tok, ) self.assertEqual(channel.code, 200, channel.json_body) - # Try with `not_tags=foo` + # Try finding rooms without the "foo" tag sync_body = { "lists": { "foo-list": { @@ -1798,11 +1881,11 @@ def test_filters_not_tags(self) -> None: response_body, _ = self.do_sync(sync_body, tok=user1_tok) self.assertIncludes( set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), - {untagged_room_id, bartag_room_id}, + {untagged_room_id, bar_room_id}, exact=True, ) - # Try with not_tags=[foo,bar] + # Try finding rooms without either "foo" or "bar" tags sync_body = { "lists": { "foo-list": { @@ -1821,3 +1904,47 @@ def test_filters_not_tags(self) -> None: {untagged_room_id}, exact=True, ) + + # Test how it behaves when we have both `tags` and `not_tags`. + # `not_tags` should win. + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 99]], + "required_state": [], + "timeline_limit": 0, + "filters": { + "tags": ["foo"], + "not_tags": ["foo"], + }, + }, + } + } + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + # Nothing matches because nothing is both tagged with "foo" and not tagged with "foo" + self.assertIncludes( + set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), + set(), + exact=True, + ) + + # Just make sure we know what happens when you specify an empty list of not_tags + # (we should find all of the rooms) + sync_body = { + "lists": { + "foo-list": { + "ranges": [[0, 99]], + "required_state": [], + "timeline_limit": 0, + "filters": { + "not_tags": [], + }, + }, + } + } + response_body, _ = self.do_sync(sync_body, tok=user1_tok) + self.assertIncludes( + set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), + {untagged_room_id, foo_room_id, bar_room_id}, + exact=True, + ) From 7c693677c0dd6b2e6915954e155ef5aabed59a18 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 12 Sep 2024 16:28:02 -0500 Subject: [PATCH 08/11] Add test for multiple tags on a room --- .../client/sliding_sync/test_lists_filters.py | 93 +++++++++++++------ 1 file changed, 66 insertions(+), 27 deletions(-) diff --git a/tests/rest/client/sliding_sync/test_lists_filters.py b/tests/rest/client/sliding_sync/test_lists_filters.py index 4ca599a1918..16e4e8edbc2 100644 --- a/tests/rest/client/sliding_sync/test_lists_filters.py +++ b/tests/rest/client/sliding_sync/test_lists_filters.py @@ -1723,6 +1723,17 @@ def test_filters_room_types_with_remote_invite_normal_room(self) -> None: exact=True, ) + def _add_tag_to_room( + self, *, room_id: str, user_id: str, access_token: str, tag_name: str + ) -> None: + channel = self.make_request( + method="PUT", + path=f"/user/{user_id}/rooms/{room_id}/tags/{tag_name}", + content={}, + access_token=access_token, + ) + self.assertEqual(channel.code, 200, channel.json_body) + def test_filters_tags(self) -> None: """ Test `filters.tags` for rooms with given tags @@ -1733,25 +1744,39 @@ def test_filters_tags(self) -> None: # Create a room with no tags self.helper.create_room_as(user1_id, tok=user1_tok) + # Create some rooms with tags foo_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) bar_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + # Create a room without multiple tags + foobar_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) - # Add the "foo" tag to the room - channel = self.make_request( - method="PUT", - path=f"/user/{user1_id}/rooms/{foo_room_id}/tags/foo", - content={}, + # Add the "foo" tag to the foo room + self._add_tag_to_room( + room_id=foo_room_id, + user_id=user1_id, access_token=user1_tok, + tag_name="foo", ) - self.assertEqual(channel.code, 200, channel.json_body) - # Add the "bar" tag to the room - channel = self.make_request( - method="PUT", - path=f"/user/{user1_id}/rooms/{bar_room_id}/tags/bar", - content={}, + # Add the "bar" tag to the bar room + self._add_tag_to_room( + room_id=bar_room_id, + user_id=user1_id, access_token=user1_tok, + tag_name="bar", + ) + # Add both "foo" and "bar" tags to the foobar room + self._add_tag_to_room( + room_id=foobar_room_id, + user_id=user1_id, + access_token=user1_tok, + tag_name="foo", + ) + self._add_tag_to_room( + room_id=foobar_room_id, + user_id=user1_id, + access_token=user1_tok, + tag_name="bar", ) - self.assertEqual(channel.code, 200, channel.json_body) # Try finding rooms with the "foo" tag sync_body = { @@ -1769,7 +1794,7 @@ def test_filters_tags(self) -> None: response_body, _ = self.do_sync(sync_body, tok=user1_tok) self.assertIncludes( set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), - {foo_room_id}, + {foo_room_id, foobar_room_id}, exact=True, ) @@ -1789,7 +1814,7 @@ def test_filters_tags(self) -> None: response_body, _ = self.do_sync(sync_body, tok=user1_tok) self.assertIncludes( set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), - {foo_room_id, bar_room_id}, + {foo_room_id, bar_room_id, foobar_room_id}, exact=True, ) @@ -1845,25 +1870,39 @@ def test_filters_not_tags(self) -> None: # Create a room with no tags untagged_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + # Create some rooms with tags foo_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) bar_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) + # Create a room without multiple tags + foobar_room_id = self.helper.create_room_as(user1_id, tok=user1_tok) - # Add the "foo" tag to the room - channel = self.make_request( - method="PUT", - path=f"/user/{user1_id}/rooms/{foo_room_id}/tags/foo", - content={}, + # Add the "foo" tag to the foo room + self._add_tag_to_room( + room_id=foo_room_id, + user_id=user1_id, access_token=user1_tok, + tag_name="foo", ) - self.assertEqual(channel.code, 200, channel.json_body) - # Add the "bar" tag to the room - channel = self.make_request( - method="PUT", - path=f"/user/{user1_id}/rooms/{bar_room_id}/tags/bar", - content={}, + # Add the "bar" tag to the bar room + self._add_tag_to_room( + room_id=bar_room_id, + user_id=user1_id, access_token=user1_tok, + tag_name="bar", + ) + # Add both "foo" and "bar" tags to the foobar room + self._add_tag_to_room( + room_id=foobar_room_id, + user_id=user1_id, + access_token=user1_tok, + tag_name="foo", + ) + self._add_tag_to_room( + room_id=foobar_room_id, + user_id=user1_id, + access_token=user1_tok, + tag_name="bar", ) - self.assertEqual(channel.code, 200, channel.json_body) # Try finding rooms without the "foo" tag sync_body = { @@ -1945,6 +1984,6 @@ def test_filters_not_tags(self) -> None: response_body, _ = self.do_sync(sync_body, tok=user1_tok) self.assertIncludes( set(response_body["lists"]["foo-list"]["ops"][0]["room_ids"]), - {untagged_room_id, foo_room_id, bar_room_id}, + {untagged_room_id, foo_room_id, bar_room_id, foobar_room_id}, exact=True, ) From 2257a11a46deda9814b9ab43ca2d6788fd83734b Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 12 Sep 2024 16:28:17 -0500 Subject: [PATCH 09/11] Avoid fetching tags if we're not filtering for them --- synapse/handlers/sliding_sync/room_lists.py | 44 +++++++++++++-------- 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index bb1bc295f57..a0a77f02a53 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -285,9 +285,6 @@ async def _compute_interested_rooms_new_tables( ) dm_room_ids = await self._get_dm_rooms_for_user(user_id) - # Fetch the user tags for their rooms - room_tags = await self.store.get_tags_for_user(user_id) - # Handle state resets in the from -> to token range. state_reset_rooms = ( newly_left_room_map.keys() - room_membership_for_user_map.keys() @@ -337,7 +334,6 @@ async def _compute_interested_rooms_new_tables( list_config.filters, to_token, dm_room_ids, - room_tags, ) # Find which rooms are partially stated and may need to be filtered out @@ -1677,7 +1673,6 @@ async def filter_rooms_using_tables( filters: SlidingSyncConfig.SlidingSyncList.Filters, to_token: StreamToken, dm_room_ids: AbstractSet[str], - room_tags: Mapping[str, Mapping[str, JsonMapping]], ) -> Dict[str, RoomsForUserSlidingSync]: """ Filter rooms based on the sync request. @@ -1785,21 +1780,36 @@ async def filter_rooms_using_tables( # ) raise NotImplementedError() - if filters.tags is not None: + # Filter by room tags according to the users account data + if filters.tags is not None or filters.not_tags is not None: with start_active_span("filters.tags"): - filtered_room_id_set = { - room_id - for room_id in filtered_room_id_set - if set(room_tags.get(room_id, [])) & set(filters.tags) + # Fetch the user tags for their rooms + room_tags = await self.store.get_tags_for_user(user_id) + room_id_to_tag_name_set: Dict[str, Set[str]] = { + room_id: set(tags.keys()) for room_id, tags in room_tags.items() } - if filters.not_tags is not None: - with start_active_span("filters.not_tags"): - filtered_room_id_set = { - room_id - for room_id in filtered_room_id_set - if not set(room_tags.get(room_id, [])) & set(filters.not_tags) - } + if filters.tags is not None: + tags_set = set(filters.tags) + filtered_room_id_set = { + room_id + for room_id in filtered_room_id_set + # Remove rooms that don't have one of the tags in the filter + if room_id_to_tag_name_set.get(room_id, set()).intersection( + tags_set + ) + } + + if filters.not_tags is not None: + not_tags_set = set(filters.not_tags) + filtered_room_id_set = { + room_id + for room_id in filtered_room_id_set + # Remove rooms if they have any of the tags in the filter + if not room_id_to_tag_name_set.get(room_id, set()).intersection( + not_tags_set + ) + } # Assemble a new sync room map but only with the `filtered_room_id_set` return {room_id: sync_room_map[room_id] for room_id in filtered_room_id_set} From 4369655e68aad4e168e7760c532e9eadd513f371 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 12 Sep 2024 16:31:14 -0500 Subject: [PATCH 10/11] Add support to the old path --- synapse/handlers/sliding_sync/room_lists.py | 31 ++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index a0a77f02a53..02a812fc905 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -1525,6 +1525,8 @@ async def filter_rooms( A filtered dictionary of room IDs along with membership information in the room at the time of `to_token`. """ + user_id = user.to_string() + room_id_to_stripped_state_map: Dict[ str, Optional[StateMap[StrippedStateEvent]] ] = {} @@ -1658,9 +1660,36 @@ async def filter_rooms( # ) raise NotImplementedError() + # Filter by room tags according to the users account data if filters.tags is not None or filters.not_tags is not None: with start_active_span("filters.tags"): - raise NotImplementedError() + # Fetch the user tags for their rooms + room_tags = await self.store.get_tags_for_user(user_id) + room_id_to_tag_name_set: Dict[str, Set[str]] = { + room_id: set(tags.keys()) for room_id, tags in room_tags.items() + } + + if filters.tags is not None: + tags_set = set(filters.tags) + filtered_room_id_set = { + room_id + for room_id in filtered_room_id_set + # Remove rooms that don't have one of the tags in the filter + if room_id_to_tag_name_set.get(room_id, set()).intersection( + tags_set + ) + } + + if filters.not_tags is not None: + not_tags_set = set(filters.not_tags) + filtered_room_id_set = { + room_id + for room_id in filtered_room_id_set + # Remove rooms if they have any of the tags in the filter + if not room_id_to_tag_name_set.get(room_id, set()).intersection( + not_tags_set + ) + } # Assemble a new sync room map but only with the `filtered_room_id_set` return {room_id: sync_room_map[room_id] for room_id in filtered_room_id_set} From 68050e747348dda94ef6db6ce245c35d2e57b446 Mon Sep 17 00:00:00 2001 From: Eric Eastwood Date: Thu, 12 Sep 2024 16:34:25 -0500 Subject: [PATCH 11/11] Fix lints --- synapse/handlers/sliding_sync/room_lists.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/handlers/sliding_sync/room_lists.py b/synapse/handlers/sliding_sync/room_lists.py index 02a812fc905..475bfbbbcb9 100644 --- a/synapse/handlers/sliding_sync/room_lists.py +++ b/synapse/handlers/sliding_sync/room_lists.py @@ -54,7 +54,6 @@ RoomsForUserStateReset, ) from synapse.types import ( - JsonMapping, MutableStateMap, PersistedEventPosition, RoomStreamToken,