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

Support the stable endpoint from MSC2946 #11329

Merged
merged 9 commits into from
Nov 29, 2021
Merged
2 changes: 1 addition & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ jobs:
working-directory: complement/dockerfiles

# Run Complement
- run: go test -v -tags synapse_blacklist,msc2403,msc2946 ./tests/...
- run: go test -v -tags synapse_blacklist,msc2403 ./tests/...
env:
COMPLEMENT_BASE_IMAGE: complement-synapse:latest
working-directory: complement
Expand Down
1 change: 1 addition & 0 deletions changelog.d/11329.feature
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Support the stable API endpoints for [MSC2946](https://github.com/matrix-org/matrix-doc/pull/2946): the room `/hierarchy` endpoint.
4 changes: 2 additions & 2 deletions docs/workers.md
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ expressions:
^/_matrix/federation/v1/get_groups_publicised$
^/_matrix/key/v2/query
^/_matrix/federation/unstable/org.matrix.msc2946/spaces/
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this one get v1 too in that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this endpoint was not stabilized and will be removed in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! It was still being hit fairly actively on matrix.org last time I checked unfortunately. It was essentially replaced with the /hierarchy version.

#10946 tracks this work for cross-referencing.

^/_matrix/federation/unstable/org.matrix.msc2946/hierarchy/
^/_matrix/federation/(v1|unstable/org.matrix.msc2946)/hierarchy/
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved

# Inbound federation transaction request
^/_matrix/federation/v1/send/
Expand All @@ -223,7 +223,7 @@ expressions:
^/_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/unstable/org.matrix.msc2946/rooms/.*/hierarchy$
^/_matrix/client/(v1|unstable/org.matrix.msc2946)/rooms/.*/hierarchy$
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
^/_matrix/client/unstable/im.nheko.summary/rooms/.*/summary$
^/_matrix/client/(api/v1|r0|v3|unstable)/account/3pid$
^/_matrix/client/(api/v1|r0|v3|unstable)/devices$
Expand Down
2 changes: 1 addition & 1 deletion scripts-dev/complement.sh
Original file line number Diff line number Diff line change
Expand Up @@ -65,4 +65,4 @@ if [[ -n "$1" ]]; then
fi

# Run the tests!
go test -v -tags synapse_blacklist,msc2946,msc2403 -count=1 $EXTRA_COMPLEMENT_ARGS ./tests/...
go test -v -tags synapse_blacklist,msc2403 -count=1 $EXTRA_COMPLEMENT_ARGS ./tests/...
1 change: 1 addition & 0 deletions synapse/app/homeserver.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ def _configure_named_resource(
{
"/_matrix/client/api/v1": client_resource,
"/_matrix/client/r0": client_resource,
"/_matrix/client/v1": client_resource,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the generic worker it seems we just put client_resource under /_matrix/client once. I'm not sure why we don't do that here too?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's because of WEB_CLIENT_PREFIX, down at line 274.

Copy link
Member

Choose a reason for hiding this comment

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

(which is not to say there aren't way better ways of doing this and I would love it if this stuff were less of a mess. see also #5118)

"/_matrix/client/v3": client_resource,
"/_matrix/client/unstable": client_resource,
"/_matrix/client/v2_alpha": client_resource,
Expand Down
31 changes: 26 additions & 5 deletions synapse/federation/federation_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -1395,11 +1395,28 @@ async def get_room_hierarchy(
async def send_request(
destination: str,
) -> Tuple[JsonDict, Sequence[JsonDict], Sequence[str]]:
res = await self.transport_layer.get_room_hierarchy(
destination=destination,
room_id=room_id,
suggested_only=suggested_only,
)
try:
res = await self.transport_layer.get_room_hierarchy(
destination=destination,
room_id=room_id,
suggested_only=suggested_only,
)
except HttpResponseException as e:
# If an error is received that is due to an unrecognised endpoint,
# fallback to the unstable endpoint. Otherwise consider it a
# legitmate error and raise.
if not self._is_unknown_endpoint(e):
raise

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

res = await self.transport_layer.get_room_hierarchy_unstable(
destination=destination,
room_id=room_id,
suggested_only=suggested_only,
)
Comment on lines +1398 to +1419
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same pattern we do with e.g. send_join.


room = res.get("room")
if not isinstance(room, dict):
Expand Down Expand Up @@ -1449,6 +1466,10 @@ async def send_request(
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.
#
Expand Down
22 changes: 18 additions & 4 deletions synapse/federation/transport/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -1192,10 +1192,24 @@ async def get_space_summary(
)

async def get_room_hierarchy(
self,
destination: str,
room_id: str,
suggested_only: bool,
self, destination: str, room_id: str, suggested_only: bool
) -> JsonDict:
Comment on lines 1194 to +1196
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could choose whether to v1 vs. unstable based on a parameter but the other bits in this file seem to have separate methods for v1 vs. v2, etc. so I followed that pattern. I think we can clean that up separately if it makes sense?

"""
Args:
destination: The remote server
room_id: The room ID to ask about.
suggested_only: if True, only suggested rooms will be returned
"""
path = _create_v1_path("/hierarchy/%s", room_id)

return await self.client.get_json(
destination=destination,
path=path,
args={"suggested_only": "true" if suggested_only else "false"},
)

async def get_room_hierarchy_unstable(
self, destination: str, room_id: str, suggested_only: bool
) -> JsonDict:
"""
Args:
Expand Down
6 changes: 5 additions & 1 deletion synapse/federation/transport/server/federation.py
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,6 @@ async def on_POST(


class FederationRoomHierarchyServlet(BaseFederationServlet):
PREFIX = FEDERATION_UNSTABLE_PREFIX + "/org.matrix.msc2946"
PATH = "/hierarchy/(?P<room_id>[^/]*)"

def __init__(
Expand All @@ -637,6 +636,10 @@ async def on_GET(
)


class FederationRoomHierarchyUnstableServlet(FederationRoomHierarchyServlet):
PREFIX = FEDERATION_UNSTABLE_PREFIX + "/org.matrix.msc2946"


class RoomComplexityServlet(BaseFederationServlet):
"""
Indicates to other servers how complex (and therefore likely
Expand Down Expand Up @@ -701,6 +704,7 @@ async def on_GET(
RoomComplexityServlet,
FederationSpaceSummaryServlet,
FederationRoomHierarchyServlet,
FederationRoomHierarchyUnstableServlet,
FederationV1SendKnockServlet,
FederationMakeKnockServlet,
)
14 changes: 10 additions & 4 deletions synapse/handlers/room_summary.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@
SynapseError,
UnsupportedRoomVersionError,
)
from synapse.api.ratelimiting import Ratelimiter
from synapse.events import EventBase
from synapse.types import JsonDict
from synapse.types import JsonDict, Requester
from synapse.util.caches.response_cache import ResponseCache

if TYPE_CHECKING:
Expand Down Expand Up @@ -93,6 +94,9 @@ def __init__(self, hs: "HomeServer"):
self._event_serializer = hs.get_event_client_serializer()
self._server_name = hs.hostname
self._federation_client = hs.get_federation_client()
self._ratelimiter = Ratelimiter(
store=self._store, clock=hs.get_clock(), rate_hz=5, burst_count=10
Copy link
Contributor

Choose a reason for hiding this comment

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

Where did these numbers come from, out of interest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made them up, they should allow for 5 queries per second without enforcing any ratelimiting until 10 queries. This might be a bit high, but I'm never quite sure what to put here and usually err on the side of allowing more. 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. We can always tighten the limits!

(Though there's a part of me that wonders if rate limiting ought to be handled by the load balancer to keep Synapse itself simpler.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some rate limits can be tweaked by the Synapse config, FWIW. They can also depend on particular data, e.g. you could limit the message rate per user, per room, per message type (or something like that).

)

# If a user tries to fetch the same page multiple times in quick succession,
# only process the first attempt and return its result to subsequent requests.
Expand Down Expand Up @@ -249,7 +253,7 @@ async def get_space_summary(

async def get_room_hierarchy(
self,
requester: str,
requester: Requester,
requested_room_id: str,
suggested_only: bool = False,
max_depth: Optional[int] = None,
Expand All @@ -276,22 +280,24 @@ async def get_room_hierarchy(
Returns:
The JSON hierarchy dictionary.
"""
await self._ratelimiter.ratelimit(requester)

# If a user tries to fetch the same page multiple times in quick succession,
# only process the first attempt and return its result to subsequent requests.
#
# This is due to the pagination process mutating internal state, attempting
# to process multiple requests for the same page will result in errors.
return await self._pagination_response_cache.wrap(
(
requester,
requester.user.to_string(),
requested_room_id,
suggested_only,
max_depth,
limit,
from_token,
),
self._get_room_hierarchy,
requester,
requester.user.to_string(),
requested_room_id,
suggested_only,
max_depth,
Expand Down
8 changes: 4 additions & 4 deletions synapse/rest/client/room.py
Original file line number Diff line number Diff line change
Expand Up @@ -1138,12 +1138,12 @@ async def on_POST(


class RoomHierarchyRestServlet(RestServlet):
PATTERNS = (
PATTERNS = [
re.compile(
"^/_matrix/client/unstable/org.matrix.msc2946"
"^/_matrix/client/(v1|unstable/org.matrix.msc2946)"
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
"/rooms/(?P<room_id>[^/]*)/hierarchy$"
),
)
]

def __init__(self, hs: "HomeServer"):
super().__init__()
Expand All @@ -1168,7 +1168,7 @@ async def on_GET(
)

return 200, await self._room_summary_handler.get_room_hierarchy(
requester.user.to_string(),
requester,
room_id,
suggested_only=parse_boolean(request, "suggested_only", default=False),
max_depth=max_depth,
Expand Down