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

Avoid sharing room hierarchy responses between users #11355

Merged
merged 3 commits into from
Nov 16, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 55 additions & 0 deletions tests/handlers/test_room_summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
from typing import Any, Iterable, List, Optional, Tuple
from unittest import mock

from twisted.internet.defer import ensureDeferred

from synapse.api.constants import (
EventContentFields,
EventTypes,
Expand Down Expand Up @@ -316,6 +318,59 @@ def test_visibility(self):
AuthError,
)

def test_room_hierarchy_cache(self) -> None:
"""In-flight room hierarchy requests are deduplicated."""
# Run two `get_room_hierarchy` calls up until they block.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"up until they block" is why we manually call ensureDeferred instead of letting get_success do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, otherwise they'd run sequentially and the cache wouldn't do anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I thought, just double checking!

deferred1 = ensureDeferred(
self.handler.get_room_hierarchy(self.user, self.space)
)
deferred2 = ensureDeferred(
self.handler.get_room_hierarchy(self.user, self.space)
)

# Complete the two calls.
result1 = self.get_success(deferred1)
result2 = self.get_success(deferred2)

# Both `get_room_hierarchy` calls should return the same result.
expected = [(self.space, [self.room]), (self.room, ())]
self._assert_hierarchy(result1, expected)
self._assert_hierarchy(result2, expected)
self.assertIs(result1, result2)

# A subsequent `get_room_hierarchy` call should reuse the result.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we testing that it re-uses it? We assert that result1 != result3 so I'm not sure what we're checking here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, that comment is missing a not.

result3 = self.get_success(
self.handler.get_room_hierarchy(self.user, self.space)
)
self._assert_hierarchy(result3, expected)
self.assertIsNot(result1, result3)

def test_room_hierarchy_cache_sharing(self) -> None:
"""Room hierarchy responses for different users are not shared."""
user2 = self.register_user("user2", "pass")

# Make the room within the space invite-only.
self.helper.send_state(
self.room,
event_type=EventTypes.JoinRules,
body={"join_rule": JoinRules.INVITE},
tok=self.token,
)

# Run two `get_room_hierarchy` calls for different users up until they block.
deferred1 = ensureDeferred(
self.handler.get_room_hierarchy(self.user, self.space)
)
deferred2 = ensureDeferred(self.handler.get_room_hierarchy(user2, self.space))

# Complete the two calls.
result1 = self.get_success(deferred1)
result2 = self.get_success(deferred2)

# The `get_room_hierarchy` calls should return different results.
self._assert_hierarchy(result1, [(self.space, [self.room]), (self.room, ())])
self._assert_hierarchy(result2, [(self.space, [self.room])])

def _create_room_with_join_rule(
self, join_rule: str, room_version: Optional[str] = None, **extra_content
) -> str:
Expand Down