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

Support spaces with > 50 rooms in the /hierarchy endpoint #11695

Merged
merged 5 commits into from
Jan 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions changelog.d/11695.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
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.
30 changes: 22 additions & 8 deletions synapse/handlers/room_summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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(
Expand Down Expand Up @@ -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,
)

Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -583,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
):
Comment on lines +593 to +595
Copy link
Member Author

Choose a reason for hiding this comment

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

This changes the /hierarchy federation API in a not great way, it would now return:

  • The requested room with all of the children state (so references to potentially unlimited).
  • The summary of the first 50 rooms.

This would cause the requesting server to query an unknown children rooms individually over federation.

The spec doesn't mention anything about this, but the MSC does:

Similarly, if a server-set limit on the size of the response is reached, additional rooms are not added to the response and can be queried individually.

room_id = child_room.get("state_key")
assert isinstance(room_id, str)
# If the room is unknown, skip it.
Expand Down Expand Up @@ -633,8 +642,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.
Expand All @@ -656,8 +665,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] = [
{
Expand All @@ -668,7 +682,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)

Expand Down
32 changes: 32 additions & 0 deletions tests/handlers/test_room_summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,38 @@ 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)

# 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):
"""A user not in a space cannot inspect it."""
user2 = self.register_user("user2", "pass")
Expand Down