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

Remove the unstable /spaces endpoint. #12073

Merged
merged 9 commits into from
Feb 28, 2022
Merged

Remove the unstable /spaces endpoint. #12073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cdc84d0
Remove Server-Server fallback to /spaces from /hierarchy.
clokep Nov 17, 2021
9ddf1d1
Remove Client-Server API for /spaces.
clokep Nov 17, 2021
cad9275
Remove Server-Server API for /spaces.
clokep Nov 17, 2021
3cc00c8
Refactoring.
clokep Nov 17, 2021
4ad4cde
Remove allowed_spaces field.
clokep Feb 24, 2022
86e4fed
The max_children parameter is no longer needed.
clokep Feb 24, 2022
8a4138b
Newsfragment
clokep Feb 24, 2022
01401a0
Remove transport support for spaces summary.
clokep Feb 28, 2022
9bd4388
Merge remote-tracking branch 'origin/develop' into clokep/stable-spac…
clokep Feb 28, 2022
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/12073.removal
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Remove the unstable `/spaces` endpoint from [MSC2946](https://github.com/matrix-org/matrix-doc/pull/2946).
2 changes: 0 additions & 2 deletions docs/workers.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,6 @@ information.
^/_matrix/federation/v1/user/devices/
^/_matrix/federation/v1/get_groups_publicised$
^/_matrix/key/v2/query
^/_matrix/federation/unstable/org.matrix.msc2946/spaces/
^/_matrix/federation/(v1|unstable/org.matrix.msc2946)/hierarchy/

# Inbound federation transaction request
Expand All @@ -225,7 +224,6 @@ information.
^/_matrix/client/(api/v1|r0|v3|unstable)/rooms/.*/context/.*$
^/_matrix/client/(api/v1|r0|v3|unstable)/rooms/.*/members$
^/_matrix/client/(api/v1|r0|v3|unstable)/rooms/.*/state$
^/_matrix/client/unstable/org.matrix.msc2946/rooms/.*/spaces$
^/_matrix/client/(v1|unstable/org.matrix.msc2946)/rooms/.*/hierarchy$
^/_matrix/client/unstable/im.nheko.summary/rooms/.*/summary$
^/_matrix/client/(r0|v3|unstable)/account/3pid$
Expand Down
226 changes: 32 additions & 194 deletions synapse/federation/federation_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1362,61 +1362,6 @@ async def get_room_complexity(
# server doesn't give it to us.
return None

async def get_space_summary(
self,
destinations: Iterable[str],
room_id: str,
suggested_only: bool,
max_rooms_per_space: Optional[int],
exclude_rooms: List[str],
) -> "FederationSpaceSummaryResult":
"""
Call other servers to get a summary of the given space


Args:
destinations: The remote servers. We will try them in turn, omitting any
that have been blacklisted.

room_id: ID of the space to be queried

suggested_only: If true, ask the remote server to only return children
with the "suggested" flag set

max_rooms_per_space: A limit on the number of children to return for each
space

exclude_rooms: A list of room IDs to tell the remote server to skip

Returns:
a parsed FederationSpaceSummaryResult

Raises:
SynapseError if we were unable to get a valid summary from any of the
remote servers
"""

async def send_request(destination: str) -> FederationSpaceSummaryResult:
res = await self.transport_layer.get_space_summary(
clokep marked this conversation as resolved.
Show resolved Hide resolved
destination=destination,
room_id=room_id,
suggested_only=suggested_only,
max_rooms_per_space=max_rooms_per_space,
exclude_rooms=exclude_rooms,
)

try:
return FederationSpaceSummaryResult.from_json_dict(res)
except ValueError as e:
raise InvalidResponseError(str(e))

return await self._try_destination_list(
"fetch space summary",
destinations,
send_request,
failover_on_unknown_endpoint=True,
)

async def get_room_hierarchy(
self,
destinations: Iterable[str],
Expand Down Expand Up @@ -1488,10 +1433,8 @@ async def send_request(
if any(not isinstance(e, dict) for e in children_state):
raise InvalidResponseError("Invalid event in 'children_state' list")
try:
[
FederationSpaceSummaryEventResult.from_json_dict(e)
for e in children_state
]
for child_state in children_state:
_validate_hierarchy_event(child_state)
except ValueError as e:
raise InvalidResponseError(str(e))

Expand All @@ -1513,62 +1456,12 @@ async def send_request(

return room, children_state, children, inaccessible_children

try:
result = await self._try_destination_list(
"fetch room hierarchy",
destinations,
send_request,
failover_on_unknown_endpoint=True,
)
except SynapseError as e:
# If an unexpected error occurred, re-raise it.
if e.code != 502:
raise

logger.debug(
"Couldn't fetch room hierarchy, falling back to the spaces API"
)

# 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
# parse information for the requested room, but the legacy API may
# return additional layers.
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)

# 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.
result = (requested_room, children_events, children, ())
result = await self._try_destination_list(
"fetch room hierarchy",
destinations,
send_request,
failover_on_unknown_endpoint=True,
)

# Cache the result to avoid fetching data over federation every time.
self._get_room_hierarchy_cache[(room_id, suggested_only)] = result
Expand Down Expand Up @@ -1710,89 +1603,34 @@ def from_json_dict(cls, d: JsonDict) -> "TimestampToEventResponse":
return cls(event_id, origin_server_ts, d)


@attr.s(frozen=True, slots=True, auto_attribs=True)
class FederationSpaceSummaryEventResult:
"""Represents a single event in the result of a successful get_space_summary call.

It's essentially just a serialised event object, but we do a bit of parsing and
validation in `from_json_dict` and store some of the validated properties in
object attributes.
"""

event_type: str
room_id: str
state_key: str
via: Sequence[str]

# the raw data, including the above keys
data: JsonDict

@classmethod
def from_json_dict(cls, d: JsonDict) -> "FederationSpaceSummaryEventResult":
"""Parse an event within the result of a /spaces/ request

Args:
d: json object to be parsed

Raises:
ValueError if d is not a valid event
"""

event_type = d.get("type")
if not isinstance(event_type, str):
raise ValueError("Invalid event: 'event_type' must be a str")

room_id = d.get("room_id")
if not isinstance(room_id, str):
raise ValueError("Invalid event: 'room_id' must be a str")

state_key = d.get("state_key")
if not isinstance(state_key, str):
raise ValueError("Invalid event: 'state_key' must be a str")
def _validate_hierarchy_event(d: JsonDict) -> None:
"""Validate an event within the result of a /hierarchy request

content = d.get("content")
if not isinstance(content, dict):
raise ValueError("Invalid event: 'content' must be a dict")
Args:
d: json object to be parsed

via = content.get("via")
if not isinstance(via, Sequence):
raise ValueError("Invalid event: 'via' must be a list")
if any(not isinstance(v, str) for v in via):
raise ValueError("Invalid event: 'via' must be a list of strings")

return cls(event_type, room_id, state_key, via, d)


@attr.s(frozen=True, slots=True, auto_attribs=True)
class FederationSpaceSummaryResult:
"""Represents the data returned by a successful get_space_summary call."""
Raises:
ValueError if d is not a valid event
"""

rooms: List[JsonDict]
events: Sequence[FederationSpaceSummaryEventResult]
event_type = d.get("type")
if not isinstance(event_type, str):
raise ValueError("Invalid event: 'event_type' must be a str")

@classmethod
def from_json_dict(cls, d: JsonDict) -> "FederationSpaceSummaryResult":
"""Parse the result of a /spaces/ request
room_id = d.get("room_id")
if not isinstance(room_id, str):
raise ValueError("Invalid event: 'room_id' must be a str")

Args:
d: json object to be parsed
state_key = d.get("state_key")
if not isinstance(state_key, str):
raise ValueError("Invalid event: 'state_key' must be a str")

Raises:
ValueError if d is not a valid /spaces/ response
"""
rooms = d.get("rooms")
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")

events = d.get("events")
if not isinstance(events, Sequence):
raise ValueError("'events' must be a list")
if any(not isinstance(e, dict) for e in events):
raise ValueError("Invalid event in 'events' list")
parsed_events = [
FederationSpaceSummaryEventResult.from_json_dict(e) for e in events
]
content = d.get("content")
if not isinstance(content, dict):
raise ValueError("Invalid event: 'content' must be a dict")

return cls(rooms, parsed_events)
via = content.get("via")
if not isinstance(via, Sequence):
raise ValueError("Invalid event: 'via' must be a list")
if any(not isinstance(v, str) for v in via):
raise ValueError("Invalid event: 'via' must be a list of strings")
Comment on lines +1632 to +1636
Copy link
Contributor

Choose a reason for hiding this comment

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

Not something that I'd expect to be fixed in this PR but strings do count as Sequence[str]s, so we need to be a bit more specific in the first isinstance check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I can do a follow-up for that. 👍 Seems like this has been wrong since the code was introduced in #9653.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for filing the follow-up: #12123.

33 changes: 0 additions & 33 deletions synapse/federation/transport/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1179,39 +1179,6 @@ async def get_room_complexity(self, destination: str, room_id: str) -> JsonDict:

return await self.client.get_json(destination=destination, path=path)

async def get_space_summary(
self,
destination: str,
room_id: str,
suggested_only: bool,
max_rooms_per_space: Optional[int],
exclude_rooms: List[str],
) -> JsonDict:
"""
Args:
destination: The remote server
room_id: The room ID to ask about.
suggested_only: if True, only suggested rooms will be returned
max_rooms_per_space: an optional limit to the number of children to be
returned per space
exclude_rooms: a list of any rooms we can skip
"""
# TODO When switching to the stable endpoint, use GET instead of POST.
path = _create_path(
FEDERATION_UNSTABLE_PREFIX, "/org.matrix.msc2946/spaces/%s", room_id
)

params = {
"suggested_only": suggested_only,
"exclude_rooms": exclude_rooms,
}
if max_rooms_per_space is not None:
params["max_rooms_per_space"] = max_rooms_per_space

return await self.client.post_json(
destination=destination, path=path, data=params
)

async def get_room_hierarchy(
self, destination: str, room_id: str, suggested_only: bool
) -> JsonDict:
Expand Down
Loading