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

Query legacy spaces federation API if the hierarchy API is unavailable. #10583

Merged
merged 1 commit into from
Aug 17, 2021
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/10583.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Add pagination to the spaces summary based on updates to [MSC2946](https://github.com/matrix-org/matrix-doc/pull/2946).
64 changes: 55 additions & 9 deletions synapse/federation/federation_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -1364,13 +1364,59 @@ async def send_request(

return room, children, inaccessible_children

# TODO Fallback to the old federation API and translate the results.
return await self._try_destination_list(
"fetch room hierarchy",
destinations,
send_request,
failover_on_unknown_endpoint=True,
)
try:
return await self._try_destination_list(
"fetch room hierarchy",
destinations,
send_request,
failover_on_unknown_endpoint=True,
)
except SynapseError as e:
# Fallback to the old federation API and translate the results if
# no servers implement the new API.
#
# The algorithm below is a bit inefficient as it only attempts to
# get information for the requested room, but the legacy API may
# return additional layers.
if e.code == 502:
Copy link
Member

Choose a reason for hiding this comment

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

Does Synapse not return a 400, M_UNRECOGNIZED for endpoints it fails to understand, rather than a 502?

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 is from

raise SynapseError(502, "Failed to %s via any server" % (description,))

Copy link
Member

Choose a reason for hiding this comment

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

aha, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

The goal is only to run this if we were unable to fetch data from all destinations, not just if we were unable to fetch it from the first destination. (This is different than our fallback of v2 -> v1 for e.g. send_join, which we do on a per-destination basis).

legacy_result = await self.get_space_summary(
destinations,
room_id,
suggested_only,
max_rooms_per_space=None,
exclude_rooms=[],
)

# Find the requested room in the response (and remove it).
for _i, room in enumerate(legacy_result.rooms):
if room.get("room_id") == room_id:
break
else:
# The requested room was not returned, nothing we can do.
raise
requested_room = legacy_result.rooms.pop(_i)

# Find any children events of the requested room.
children_events = []
children_room_ids = set()
for event in legacy_result.events:
if event.room_id == room_id:
children_events.append(event.data)
children_room_ids.add(event.state_key)
# And add them under the requested room.
requested_room["children_state"] = children_events

# Find the children rooms.
children = []
for room in legacy_result.rooms:
if room.get("room_id") in children_room_ids:
children.append(room)

# It isn't clear from the response whether some of the rooms are
# not accessible.
return requested_room, children, ()
Comment on lines +1415 to +1417
Copy link
Member

Choose a reason for hiding this comment

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

Will this produce odd results on the client-side where one may believe they can access a room, but will find they are unable to?

Mostly a question out of curiosity, as I don't know any way that the situation could be improved (and this is a legacy API so meh).

Copy link
Member Author

Choose a reason for hiding this comment

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

It shouldn't, this is before any filtering is done on whether a room is accessible or not, it pretty much is just about changing API shapes from the old /spaces endpoint to the new /hierarchy. Part of that is that the /spaces endpoint returned multiple layers deep (over federation) while the /hierarchy one does not. So we throw away a bunch of data.

The overall algorithm is:

  1. Find the requested room (which is returned in rooms for /spaces, but as a separate field for /hierarchy)
  2. Attach the children events to it (which is returned in a list including children for other rooms in /spaces and as part of the room object for /hierarchy).
  3. Get the summaries for any of those children room (which are returned as rooms for /spaces, but those will include additional layers).

Copy link
Member

Choose a reason for hiding this comment

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

Aha, I see. Thanks for explaining it so clearly.


raise


@attr.s(frozen=True, slots=True, auto_attribs=True)
Expand Down Expand Up @@ -1430,7 +1476,7 @@ def from_json_dict(cls, d: JsonDict) -> "FederationSpaceSummaryEventResult":
class FederationSpaceSummaryResult:
"""Represents the data returned by a successful get_space_summary call."""

rooms: Sequence[JsonDict]
rooms: List[JsonDict]
events: Sequence[FederationSpaceSummaryEventResult]

@classmethod
Expand All @@ -1444,7 +1490,7 @@ def from_json_dict(cls, d: JsonDict) -> "FederationSpaceSummaryResult":
ValueError if d is not a valid /spaces/ response
"""
rooms = d.get("rooms")
if not isinstance(rooms, Sequence):
if not isinstance(rooms, List):
raise ValueError("'rooms' must be a list")
if any(not isinstance(r, dict) for r in rooms):
raise ValueError("Invalid room in 'rooms' list")
Expand Down