Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Commit

Permalink
Remove cache for get_shared_rooms_for_users (#9416)
Browse files Browse the repository at this point in the history
This PR remove the cache for the `get_shared_rooms_for_users` storage method (the db method driving the experimental "what rooms do I share with this user?" feature: [MSC2666](matrix-org/matrix-spec-proposals#2666)). Currently subsequent requests to the endpoint will return the same result, even if your shared rooms with that user have changed.

The cache was added in #7785, but we forgot to ensure it was invalidated appropriately.

Upon attempting to invalidate it, I found that the cache had to be entirely invalidated whenever a user (remote or local) joined or left a room. This didn't make for a very useful cache, especially for a function that may or may not be called very often. Thus, I've opted to remove it instead of invalidating it.
  • Loading branch information
anoadragon453 authored Feb 22, 2021
1 parent e22b718 commit 0a363f9
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 37 deletions.
1 change: 1 addition & 0 deletions changelog.d/9416.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug that caused multiple calls to the experimental `shared_rooms` endpoint to return stale results.
4 changes: 1 addition & 3 deletions synapse/storage/databases/main/user_directory.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -670,7 +669,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]:
Expand Down
75 changes: 41 additions & 34 deletions tests/rest/client/v2_alpha/test_shared_rooms.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
"""
Expand All @@ -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)

0 comments on commit 0a363f9

Please sign in to comment.