From de45532aa75e741f87dc5c402f74eda43cb756ee Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 6 Jan 2022 10:16:07 -0500 Subject: [PATCH 1/5] Add test for large spaces (>50 rooms). --- tests/handlers/test_room_summary.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tests/handlers/test_room_summary.py b/tests/handlers/test_room_summary.py index e5a6a6c747bf..52a81058b56c 100644 --- a/tests/handlers/test_room_summary.py +++ b/tests/handlers/test_room_summary.py @@ -253,6 +253,27 @@ def test_simple_space(self): ) self._assert_hierarchy(result, expected) + def test_large_space(self): + """Test a space with a large number of rooms.""" + rooms = [self.room] + # Make at least 51 rooms that are part of the space. + for _ in range(55): + room = self.helper.create_room_as(self.user, tok=self.token) + self._add_child(self.space, room, self.token) + rooms.append(room) + + result = self.get_success(self.handler.get_space_summary(self.user, self.space)) + # The spaces result should have the space and the first 50 rooms in it, + # along with the links from space -> room for those 50 rooms. + expected = [(self.space, rooms[:50])] + [(room, []) for room in rooms[:49]] + self._assert_rooms(result, expected) + + # Make two requests to fully paginate the results. + result = self.get_success( + self.handler.get_room_hierarchy(create_requester(self.user), self.space) + ) + self._assert_hierarchy(result, expected) + def test_visibility(self): """A user not in a space cannot inspect it.""" user2 = self.register_user("user2", "pass") From 697df7413d7f05ccb27dda0a561bf85614e1665f Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 6 Jan 2022 07:39:12 -0500 Subject: [PATCH 2/5] Handle > 50 rooms in a space for the /hierarchy endpoint. By hoisting the /spaces-only max-children logic into the callers. --- synapse/handlers/room_summary.py | 26 +++++++++++++++++++------- tests/handlers/test_room_summary.py | 11 +++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/synapse/handlers/room_summary.py b/synapse/handlers/room_summary.py index 9ef88feb8ab7..ef93dc39337e 100644 --- a/synapse/handlers/room_summary.py +++ b/synapse/handlers/room_summary.py @@ -153,6 +153,9 @@ async def get_space_summary( rooms_result: List[JsonDict] = [] events_result: List[JsonDict] = [] + if max_rooms_per_space is None or max_rooms_per_space > MAX_ROOMS_PER_SPACE: + max_rooms_per_space = MAX_ROOMS_PER_SPACE + while room_queue and len(rooms_result) < MAX_ROOMS: queue_entry = room_queue.popleft() room_id = queue_entry.room_id @@ -167,7 +170,7 @@ async def get_space_summary( # The client-specified max_rooms_per_space limit doesn't apply to the # room_id specified in the request, so we ignore it if this is the # first room we are processing. - max_children = max_rooms_per_space if processed_rooms else None + max_children = max_rooms_per_space if processed_rooms else MAX_ROOMS if is_in_room: room_entry = await self._summarize_local_room( @@ -395,7 +398,7 @@ async def _get_room_hierarchy( None, room_id, suggested_only, - # TODO Handle max children. + # Do not limit the maximum children. max_children=None, ) @@ -525,6 +528,10 @@ async def federation_space_summary( rooms_result: List[JsonDict] = [] events_result: List[JsonDict] = [] + # Set a limit on the number of rooms to return. + if max_rooms_per_space is None or max_rooms_per_space > MAX_ROOMS_PER_SPACE: + max_rooms_per_space = MAX_ROOMS_PER_SPACE + while room_queue and len(rooms_result) < MAX_ROOMS: room_id = room_queue.popleft() if room_id in processed_rooms: @@ -633,8 +640,8 @@ async def _summarize_local_room( suggested_only: True if only suggested children should be returned. Otherwise, all children are returned. max_children: - The maximum number of children rooms to include. This is capped - to a server-set limit. + The maximum number of children rooms to include. A value of None + means no limit. Returns: A room entry if the room should be returned. None, otherwise. @@ -656,8 +663,13 @@ async def _summarize_local_room( # we only care about suggested children child_events = filter(_is_suggested_child_event, child_events) - if max_children is None or max_children > MAX_ROOMS_PER_SPACE: - max_children = MAX_ROOMS_PER_SPACE + # TODO max_children is legacy code for the /spaces endpoint. + if max_children is not None: + child_iter: Iterable[EventBase] = itertools.islice( + child_events, max_children + ) + else: + child_iter = child_events stripped_events: List[JsonDict] = [ { @@ -668,7 +680,7 @@ async def _summarize_local_room( "sender": e.sender, "origin_server_ts": e.origin_server_ts, } - for e in itertools.islice(child_events, max_children) + for e in child_iter ] return _RoomEntry(room_id, room_entry, stripped_events) diff --git a/tests/handlers/test_room_summary.py b/tests/handlers/test_room_summary.py index 52a81058b56c..ce3ebcf2f2fa 100644 --- a/tests/handlers/test_room_summary.py +++ b/tests/handlers/test_room_summary.py @@ -268,10 +268,21 @@ def test_large_space(self): expected = [(self.space, rooms[:50])] + [(room, []) for room in rooms[:49]] self._assert_rooms(result, expected) + # The result should have the space and the rooms in it, along with the links + # from space -> room. + expected = [(self.space, rooms)] + [(room, []) for room in rooms] + # Make two requests to fully paginate the results. result = self.get_success( self.handler.get_room_hierarchy(create_requester(self.user), self.space) ) + result2 = self.get_success( + self.handler.get_room_hierarchy( + create_requester(self.user), self.space, from_token=result["next_batch"] + ) + ) + # Combine the results. + result["rooms"] += result2["rooms"] self._assert_hierarchy(result, expected) def test_visibility(self): From a30458c9c23ace50b10093414048431d79657a6a Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 6 Jan 2022 10:52:21 -0500 Subject: [PATCH 3/5] Newsfragment --- changelog.d/11695.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11695.bugfix diff --git a/changelog.d/11695.bugfix b/changelog.d/11695.bugfix new file mode 100644 index 000000000000..eed3ee13d42a --- /dev/null +++ b/changelog.d/11695.bugfix @@ -0,0 +1 @@ +Fix a bug where the only the first 50 rooms from a space were return from the `/hierarchy` API. This has existed since the introduction of the API in Synapse v1.41.0. From 219e23c9d3332565a457a099db9878cbec1b7c06 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Thu, 6 Jan 2022 11:26:52 -0500 Subject: [PATCH 4/5] Limit the number of rooms sent back over federation. --- synapse/handlers/room_summary.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/synapse/handlers/room_summary.py b/synapse/handlers/room_summary.py index ef93dc39337e..7c60cb0bdd27 100644 --- a/synapse/handlers/room_summary.py +++ b/synapse/handlers/room_summary.py @@ -590,7 +590,9 @@ async def get_federation_hierarchy( # Iterate through each child and potentially add it, but not its children, # to the response. - for child_room in root_room_entry.children_state_events: + for child_room in itertools.islice( + root_room_entry.children_state_events, MAX_ROOMS_PER_SPACE + ): room_id = child_room.get("state_key") assert isinstance(room_id, str) # If the room is unknown, skip it. From 40f739dade887ae0afa0899d730156e6f80b51e7 Mon Sep 17 00:00:00 2001 From: Patrick Cloke Date: Fri, 7 Jan 2022 13:14:28 -0500 Subject: [PATCH 5/5] Fix changelog. Co-authored-by: Sean Quah <8349537+squahtx@users.noreply.github.com> --- changelog.d/11695.bugfix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog.d/11695.bugfix b/changelog.d/11695.bugfix index eed3ee13d42a..7799aefb8258 100644 --- a/changelog.d/11695.bugfix +++ b/changelog.d/11695.bugfix @@ -1 +1 @@ -Fix a bug where the only the first 50 rooms from a space were return from the `/hierarchy` API. This has existed since the introduction of the API in Synapse v1.41.0. +Fix a bug where the only the first 50 rooms from a space were returned from the `/hierarchy` API. This has existed since the introduction of the API in Synapse v1.41.0.