From f55836929d3c64f3f8d883d8f3643a88b6c9cbca Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Tue, 29 Jun 2021 12:00:04 -0400 Subject: [PATCH] Do not recurse into non-spaces in the spaces summary. (#10256) Previously m.child.room events in non-space rooms would be treated as part of the room graph, but this is no longer supported. --- changelog.d/10256.misc | 1 + synapse/api/constants.py | 6 ++++ synapse/handlers/space_summary.py | 11 +++++-- tests/handlers/test_space_summary.py | 48 +++++++++++++++------------- tests/rest/client/v1/utils.py | 3 +- 5 files changed, 43 insertions(+), 26 deletions(-) create mode 100644 changelog.d/10256.misc diff --git a/changelog.d/10256.misc b/changelog.d/10256.misc new file mode 100644 index 000000000..adef12fcb --- /dev/null +++ b/changelog.d/10256.misc @@ -0,0 +1 @@ +Improve the performance of the spaces summary endpoint by only recursing into spaces (and not rooms in general). diff --git a/synapse/api/constants.py b/synapse/api/constants.py index 414e4c019..8363c2bb0 100644 --- a/synapse/api/constants.py +++ b/synapse/api/constants.py @@ -201,6 +201,12 @@ class EventContentFields: ) +class RoomTypes: + """Understood values of the room_type field of m.room.create events.""" + + SPACE = "m.space" + + class RoomEncryptionAlgorithms: MEGOLM_V1_AES_SHA2 = "m.megolm.v1.aes-sha2" DEFAULT = MEGOLM_V1_AES_SHA2 diff --git a/synapse/handlers/space_summary.py b/synapse/handlers/space_summary.py index 17fc47ce1..266f36988 100644 --- a/synapse/handlers/space_summary.py +++ b/synapse/handlers/space_summary.py @@ -25,6 +25,7 @@ from synapse.api.constants import ( EventTypes, HistoryVisibility, Membership, + RoomTypes, ) from synapse.events import EventBase from synapse.events.utils import format_event_for_client_v2 @@ -318,7 +319,8 @@ class SpaceSummaryHandler: Returns: A tuple of: - An iterable of a single value of the room. + The room information, if the room should be returned to the + user. None, otherwise. An iterable of the sorted children events. This may be limited to a maximum size or may include all children. @@ -328,7 +330,11 @@ class SpaceSummaryHandler: room_entry = await self._build_room_entry(room_id) - # look for child rooms/spaces. + # If the room is not a space, return just the room information. + if room_entry.get("room_type") != RoomTypes.SPACE: + return room_entry, () + + # Otherwise, look for child rooms/spaces. child_events = await self._get_child_events(room_id) if suggested_only: @@ -348,6 +354,7 @@ class SpaceSummaryHandler: event_format=format_event_for_client_v2, ) ) + return room_entry, events_result async def _summarize_remote_room( diff --git a/tests/handlers/test_space_summary.py b/tests/handlers/test_space_summary.py index 131d362cc..9771d3fb3 100644 --- a/tests/handlers/test_space_summary.py +++ b/tests/handlers/test_space_summary.py @@ -14,6 +14,7 @@ from typing import Any, Iterable, Optional, Tuple from unittest import mock +from synapse.api.constants import EventContentFields, RoomTypes from synapse.api.errors import AuthError from synapse.handlers.space_summary import _child_events_comparison_key from synapse.rest import admin @@ -97,9 +98,21 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase): self.hs = hs self.handler = self.hs.get_space_summary_handler() + # Create a user. self.user = self.register_user("user", "pass") self.token = self.login("user", "pass") + # Create a space and a child room. + self.space = self.helper.create_room_as( + self.user, + tok=self.token, + extra_content={ + "creation_content": {EventContentFields.ROOM_TYPE: RoomTypes.SPACE} + }, + ) + self.room = self.helper.create_room_as(self.user, tok=self.token) + self._add_child(self.space, self.room, self.token) + def _add_child(self, space_id: str, room_id: str, token: str) -> None: """Add a child room to a space.""" self.helper.send_state( @@ -128,43 +141,32 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase): def test_simple_space(self): """Test a simple space with a single room.""" - space = self.helper.create_room_as(self.user, tok=self.token) - room = self.helper.create_room_as(self.user, tok=self.token) - self._add_child(space, room, self.token) - - result = self.get_success(self.handler.get_space_summary(self.user, space)) + result = self.get_success(self.handler.get_space_summary(self.user, self.space)) # The result should have the space and the room in it, along with a link # from space -> room. - self._assert_rooms(result, [space, room]) - self._assert_events(result, [(space, room)]) + self._assert_rooms(result, [self.space, self.room]) + self._assert_events(result, [(self.space, self.room)]) def test_visibility(self): """A user not in a space cannot inspect it.""" - space = self.helper.create_room_as(self.user, tok=self.token) - room = self.helper.create_room_as(self.user, tok=self.token) - self._add_child(space, room, self.token) - user2 = self.register_user("user2", "pass") token2 = self.login("user2", "pass") # The user cannot see the space. - self.get_failure(self.handler.get_space_summary(user2, space), AuthError) + self.get_failure(self.handler.get_space_summary(user2, self.space), AuthError) # Joining the room causes it to be visible. - self.helper.join(space, user2, tok=token2) - result = self.get_success(self.handler.get_space_summary(user2, space)) + self.helper.join(self.space, user2, tok=token2) + result = self.get_success(self.handler.get_space_summary(user2, self.space)) # The result should only have the space, but includes the link to the room. - self._assert_rooms(result, [space]) - self._assert_events(result, [(space, room)]) + self._assert_rooms(result, [self.space]) + self._assert_events(result, [(self.space, self.room)]) def test_world_readable(self): """A world-readable room is visible to everyone.""" - space = self.helper.create_room_as(self.user, tok=self.token) - room = self.helper.create_room_as(self.user, tok=self.token) - self._add_child(space, room, self.token) self.helper.send_state( - space, + self.space, event_type="m.room.history_visibility", body={"history_visibility": "world_readable"}, tok=self.token, @@ -173,6 +175,6 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase): user2 = self.register_user("user2", "pass") # The space should be visible, as well as the link to the room. - result = self.get_success(self.handler.get_space_summary(user2, space)) - self._assert_rooms(result, [space]) - self._assert_events(result, [(space, room)]) + result = self.get_success(self.handler.get_space_summary(user2, self.space)) + self._assert_rooms(result, [self.space]) + self._assert_events(result, [(self.space, self.room)]) diff --git a/tests/rest/client/v1/utils.py b/tests/rest/client/v1/utils.py index ed55a640a..69798e95c 100644 --- a/tests/rest/client/v1/utils.py +++ b/tests/rest/client/v1/utils.py @@ -52,6 +52,7 @@ class RestHelper: room_version: str = None, tok: str = None, expect_code: int = 200, + extra_content: Optional[Dict] = None, ) -> str: """ Create a room. @@ -72,7 +73,7 @@ class RestHelper: temp_id = self.auth_user_id self.auth_user_id = room_creator path = "/_matrix/client/r0/createRoom" - content = {} + content = extra_content or {} if not is_public: content["visibility"] = "private" if room_version: