From 46b58ac73b4478c9a471677a12ad4215b299a6e9 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 22 Feb 2021 15:55:25 +0000 Subject: [PATCH 1/4] Drop cache for get_shared_rooms_for_users It's not clear that the benefit of caching this function outweighs the performance cost of invalidating the cache when the underlying tables change. --- synapse/storage/databases/main/user_directory.py | 1 - 1 file changed, 1 deletion(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 02ee15676ca8..3fbc1f42e711 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -670,7 +670,6 @@ async def get_user_dir_rooms_user_is_in(self, user_id): users.update(rows) return list(users) - @cached() async def get_shared_rooms_for_users( self, user_id: str, other_user_id: str ) -> Set[str]: From 9e3f53562008e2a987941dd073514021ed1e1d4f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 22 Feb 2021 15:52:51 +0000 Subject: [PATCH 2/4] Fix docstring of add_users_in_public_rooms --- synapse/storage/databases/main/user_directory.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/user_directory.py b/synapse/storage/databases/main/user_directory.py index 3fbc1f42e711..1026f321e51e 100644 --- a/synapse/storage/databases/main/user_directory.py +++ b/synapse/storage/databases/main/user_directory.py @@ -497,8 +497,7 @@ async def add_users_who_share_private_room( async def add_users_in_public_rooms( self, room_id: str, user_ids: Iterable[str] ) -> None: - """Insert entries into the users_who_share_private_rooms table. The first - user should be a local user. + """Insert entries into the users_in_public_rooms table. Args: room_id From bfb7833eff8ddf5fd6b9372b5fdf2e077aabe420 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 22 Feb 2021 16:00:06 +0000 Subject: [PATCH 3/4] Fix tests to check the shared_rooms endpoints multiple times Beforehand, with the cache added, multiple calls would give back the same results, even if the users started sharing a new room in-between those calls. Removing the cache should fix the problem, but it's a good idea to add tests to cache the mistake in the future; in case we do decide to add a cache again. --- .../rest/client/v2_alpha/test_shared_rooms.py | 75 ++++++++++--------- 1 file changed, 41 insertions(+), 34 deletions(-) diff --git a/tests/rest/client/v2_alpha/test_shared_rooms.py b/tests/rest/client/v2_alpha/test_shared_rooms.py index 116ace1812cd..dd83a1f8ff77 100644 --- a/tests/rest/client/v2_alpha/test_shared_rooms.py +++ b/tests/rest/client/v2_alpha/test_shared_rooms.py @@ -54,61 +54,62 @@ def test_shared_room_list_public(self): A room should show up in the shared list of rooms between two users if it is public. """ - u1 = self.register_user("user1", "pass") - u1_token = self.login(u1, "pass") - u2 = self.register_user("user2", "pass") - u2_token = self.login(u2, "pass") - - room = self.helper.create_room_as(u1, is_public=True, tok=u1_token) - self.helper.invite(room, src=u1, targ=u2, tok=u1_token) - self.helper.join(room, user=u2, tok=u2_token) - - channel = self._get_shared_rooms(u1_token, u2) - self.assertEquals(200, channel.code, channel.result) - self.assertEquals(len(channel.json_body["joined"]), 1) - self.assertEquals(channel.json_body["joined"][0], room) + self._check_shared_rooms_with(room_one_is_public=True, room_two_is_public=True) def test_shared_room_list_private(self): """ A room should show up in the shared list of rooms between two users if it is private. """ - u1 = self.register_user("user1", "pass") - u1_token = self.login(u1, "pass") - u2 = self.register_user("user2", "pass") - u2_token = self.login(u2, "pass") - - room = self.helper.create_room_as(u1, is_public=False, tok=u1_token) - self.helper.invite(room, src=u1, targ=u2, tok=u1_token) - self.helper.join(room, user=u2, tok=u2_token) - - channel = self._get_shared_rooms(u1_token, u2) - self.assertEquals(200, channel.code, channel.result) - self.assertEquals(len(channel.json_body["joined"]), 1) - self.assertEquals(channel.json_body["joined"][0], room) + self._check_shared_rooms_with( + room_one_is_public=False, room_two_is_public=False + ) def test_shared_room_list_mixed(self): """ The shared room list between two users should contain both public and private rooms. """ + self._check_shared_rooms_with(room_one_is_public=True, room_two_is_public=False) + + def _check_shared_rooms_with( + self, room_one_is_public: bool, room_two_is_public: bool + ): + """Checks that shared public or private rooms between two users appear in + their shared room lists + """ u1 = self.register_user("user1", "pass") u1_token = self.login(u1, "pass") u2 = self.register_user("user2", "pass") u2_token = self.login(u2, "pass") - room_public = self.helper.create_room_as(u1, is_public=True, tok=u1_token) - room_private = self.helper.create_room_as(u2, is_public=False, tok=u2_token) - self.helper.invite(room_public, src=u1, targ=u2, tok=u1_token) - self.helper.invite(room_private, src=u2, targ=u1, tok=u2_token) - self.helper.join(room_public, user=u2, tok=u2_token) - self.helper.join(room_private, user=u1, tok=u1_token) + # Create a room. user1 invites user2, who joins + room_id_one = self.helper.create_room_as( + u1, is_public=room_one_is_public, tok=u1_token + ) + self.helper.invite(room_id_one, src=u1, targ=u2, tok=u1_token) + self.helper.join(room_id_one, user=u2, tok=u2_token) + # Check shared rooms from user1's perspective. + # We should see the one room in common + channel = self._get_shared_rooms(u1_token, u2) + self.assertEquals(200, channel.code, channel.result) + self.assertEquals(len(channel.json_body["joined"]), 1) + self.assertEquals(channel.json_body["joined"][0], room_id_one) + + # Create another room and invite user2 to it + room_id_two = self.helper.create_room_as( + u1, is_public=room_two_is_public, tok=u1_token + ) + self.helper.invite(room_id_two, src=u1, targ=u2, tok=u1_token) + self.helper.join(room_id_two, user=u2, tok=u2_token) + + # Check shared rooms again. We should now see both rooms. channel = self._get_shared_rooms(u1_token, u2) self.assertEquals(200, channel.code, channel.result) self.assertEquals(len(channel.json_body["joined"]), 2) - self.assertTrue(room_public in channel.json_body["joined"]) - self.assertTrue(room_private in channel.json_body["joined"]) + for room_id_id in channel.json_body["joined"]: + self.assertIn(room_id_id, [room_id_one, room_id_two]) def test_shared_room_list_after_leave(self): """ @@ -132,6 +133,12 @@ def test_shared_room_list_after_leave(self): self.helper.leave(room, user=u1, tok=u1_token) + # Check user1's view of shared rooms with user2 + channel = self._get_shared_rooms(u1_token, u2) + self.assertEquals(200, channel.code, channel.result) + self.assertEquals(len(channel.json_body["joined"]), 0) + + # Check user2's view of shared rooms with user1 channel = self._get_shared_rooms(u2_token, u1) self.assertEquals(200, channel.code, channel.result) self.assertEquals(len(channel.json_body["joined"]), 0) From 94022c41db2118752d4cf863a4337dad44460849 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 22 Feb 2021 16:02:13 +0000 Subject: [PATCH 4/4] Changelog --- changelog.d/9416.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/9416.bugfix diff --git a/changelog.d/9416.bugfix b/changelog.d/9416.bugfix new file mode 100644 index 000000000000..4d79cb222801 --- /dev/null +++ b/changelog.d/9416.bugfix @@ -0,0 +1 @@ +Fix a bug that caused multiple calls to the experimental `shared_rooms` endpoint to return stale results. \ No newline at end of file