Support the stable /hierarchy endpoint from MSC2946 (#11329)

This also makes additional updates where the implementation
had drifted from the approved MSC.

Unstable endpoints will be removed at a later data.

.github/workflows/tests.yml

@@ -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

changelog.d/11329.feature

@@ -0,0 +1 @@
+Support the stable API endpoints for [MSC2946](https://github.com/matrix-org/matrix-doc/pull/2946): the room `/hierarchy` endpoint.

docs/workers.md

@@ -210,7 +210,7 @@ expressions:
     ^/_matrix/federation/v1/get_groups_publicised$
     ^/_matrix/key/v2/query
     ^/_matrix/federation/unstable/org.matrix.msc2946/spaces/
-    ^/_matrix/federation/unstable/org.matrix.msc2946/hierarchy/
+    ^/_matrix/federation/(v1|unstable/org.matrix.msc2946)/hierarchy/
 
     # Inbound federation transaction request
     ^/_matrix/federation/v1/send/
@@ -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$
     ^/_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$

scripts-dev/complement.sh

@@ -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/...

synapse/app/homeserver.py

@@ -194,6 +194,7 @@ class SynapseHomeServer(HomeServer):
                 {
                     "/_matrix/client/api/v1": client_resource,
                     "/_matrix/client/r0": client_resource,
+                    "/_matrix/client/v1": client_resource,
                     "/_matrix/client/v3": client_resource,
                     "/_matrix/client/unstable": client_resource,
                     "/_matrix/client/v2_alpha": client_resource,

synapse/federation/federation_client.py

@@ -1395,11 +1395,28 @@ class FederationClient(FederationBase):
         async def send_request(
             destination: str,
         ) -> Tuple[JsonDict, Sequence[JsonDict], Sequence[str]]:
-            res = await self.transport_layer.get_room_hierarchy(
+            try:
-                destination=destination,
+                res = await self.transport_layer.get_room_hierarchy(
-                room_id=room_id,
+                    destination=destination,
-                suggested_only=suggested_only,
+                    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,
+                )
 
             room = res.get("room")
             if not isinstance(room, dict):
@@ -1449,6 +1466,10 @@ class FederationClient(FederationBase):
             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.
             #

synapse/federation/transport/client.py

@@ -1192,10 +1192,24 @@ class TransportLayerClient:
         )
 
     async def get_room_hierarchy(
-        self,
+        self, destination: str, room_id: str, suggested_only: bool
-        destination: str,
+    ) -> JsonDict:
-        room_id: str,
+        """
-        suggested_only: bool,
+        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:

synapse/federation/transport/server/federation.py

@@ -611,7 +611,6 @@ class FederationSpaceSummaryServlet(BaseFederationServlet):
 
 
 class FederationRoomHierarchyServlet(BaseFederationServlet):
-    PREFIX = FEDERATION_UNSTABLE_PREFIX + "/org.matrix.msc2946"
     PATH = "/hierarchy/(?P<room_id>[^/]*)"
 
     def __init__(
@@ -637,6 +636,10 @@ class FederationRoomHierarchyServlet(BaseFederationServlet):
         )
 
 
+class FederationRoomHierarchyUnstableServlet(FederationRoomHierarchyServlet):
+    PREFIX = FEDERATION_UNSTABLE_PREFIX + "/org.matrix.msc2946"
+
+
 class RoomComplexityServlet(BaseFederationServlet):
     """
     Indicates to other servers how complex (and therefore likely
@@ -701,6 +704,7 @@ FEDERATION_SERVLET_CLASSES: Tuple[Type[BaseFederationServlet], ...] = (
     RoomComplexityServlet,
     FederationSpaceSummaryServlet,
     FederationRoomHierarchyServlet,
+    FederationRoomHierarchyUnstableServlet,
     FederationV1SendKnockServlet,
     FederationMakeKnockServlet,
 )

synapse/handlers/room_summary.py

@@ -36,8 +36,9 @@ from synapse.api.errors import (
     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:
@@ -93,6 +94,9 @@ class RoomSummaryHandler:
         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
+        )
 
         # 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.
@@ -249,7 +253,7 @@ class RoomSummaryHandler:
 
     async def get_room_hierarchy(
         self,
-        requester: str,
+        requester: Requester,
         requested_room_id: str,
         suggested_only: bool = False,
         max_depth: Optional[int] = None,
@@ -276,6 +280,8 @@ class RoomSummaryHandler:
         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.
         #
@@ -283,7 +289,7 @@ class RoomSummaryHandler:
         # 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,
@@ -291,7 +297,7 @@ class RoomSummaryHandler:
                 from_token,
             ),
             self._get_room_hierarchy,
-            requester,
+            requester.user.to_string(),
             requested_room_id,
             suggested_only,
             max_depth,

synapse/rest/client/room.py

@@ -1138,12 +1138,12 @@ class RoomSpaceSummaryRestServlet(RestServlet):
 
 
 class RoomHierarchyRestServlet(RestServlet):
-    PATTERNS = (
+    PATTERNS = [
         re.compile(
-            "^/_matrix/client/unstable/org.matrix.msc2946"
+            "^/_matrix/client/(v1|unstable/org.matrix.msc2946)"
             "/rooms/(?P<room_id>[^/]*)/hierarchy$"
         ),
-    )
+    ]
 
     def __init__(self, hs: "HomeServer"):
         super().__init__()
@@ -1168,7 +1168,7 @@ class RoomHierarchyRestServlet(RestServlet):
             )
 
         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,

tests/handlers/test_room_summary.py

@@ -32,7 +32,7 @@ from synapse.handlers.room_summary import _child_events_comparison_key, _RoomEnt
 from synapse.rest import admin
 from synapse.rest.client import login, room
 from synapse.server import HomeServer
-from synapse.types import JsonDict, UserID
+from synapse.types import JsonDict, UserID, create_requester
 
 from tests import unittest
 
@@ -249,7 +249,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         self._assert_rooms(result, expected)
 
         result = self.get_success(
-            self.handler.get_room_hierarchy(self.user, self.space)
+            self.handler.get_room_hierarchy(create_requester(self.user), self.space)
         )
         self._assert_hierarchy(result, expected)
 
@@ -263,7 +263,9 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         expected = [(self.space, [self.room]), (self.room, ())]
         self._assert_rooms(result, expected)
 
-        result = self.get_success(self.handler.get_room_hierarchy(user2, self.space))
+        result = self.get_success(
+            self.handler.get_room_hierarchy(create_requester(user2), self.space)
+        )
         self._assert_hierarchy(result, expected)
 
         # If the space is made invite-only, it should no longer be viewable.
@@ -274,7 +276,10 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
             tok=self.token,
         )
         self.get_failure(self.handler.get_space_summary(user2, self.space), AuthError)
-        self.get_failure(self.handler.get_room_hierarchy(user2, self.space), AuthError)
+        self.get_failure(
+            self.handler.get_room_hierarchy(create_requester(user2), self.space),
+            AuthError,
+        )
 
         # If the space is made world-readable it should return a result.
         self.helper.send_state(
@@ -286,7 +291,9 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         result = self.get_success(self.handler.get_space_summary(user2, self.space))
         self._assert_rooms(result, expected)
 
-        result = self.get_success(self.handler.get_room_hierarchy(user2, self.space))
+        result = self.get_success(
+            self.handler.get_room_hierarchy(create_requester(user2), self.space)
+        )
         self._assert_hierarchy(result, expected)
 
         # Make it not world-readable again and confirm it results in an error.
@@ -297,7 +304,10 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
             tok=self.token,
         )
         self.get_failure(self.handler.get_space_summary(user2, self.space), AuthError)
-        self.get_failure(self.handler.get_room_hierarchy(user2, self.space), AuthError)
+        self.get_failure(
+            self.handler.get_room_hierarchy(create_requester(user2), self.space),
+            AuthError,
+        )
 
         # Join the space and results should be returned.
         self.helper.invite(self.space, targ=user2, tok=self.token)
@@ -305,7 +315,9 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         result = self.get_success(self.handler.get_space_summary(user2, self.space))
         self._assert_rooms(result, expected)
 
-        result = self.get_success(self.handler.get_room_hierarchy(user2, self.space))
+        result = self.get_success(
+            self.handler.get_room_hierarchy(create_requester(user2), self.space)
+        )
         self._assert_hierarchy(result, expected)
 
         # Attempting to view an unknown room returns the same error.
@@ -314,7 +326,9 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
             AuthError,
         )
         self.get_failure(
-            self.handler.get_room_hierarchy(user2, "#not-a-space:" + self.hs.hostname),
+            self.handler.get_room_hierarchy(
+                create_requester(user2), "#not-a-space:" + self.hs.hostname
+            ),
             AuthError,
         )
 
@@ -322,10 +336,10 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         """In-flight room hierarchy requests are deduplicated."""
         # Run two `get_room_hierarchy` calls up until they block.
         deferred1 = ensureDeferred(
-            self.handler.get_room_hierarchy(self.user, self.space)
+            self.handler.get_room_hierarchy(create_requester(self.user), self.space)
         )
         deferred2 = ensureDeferred(
-            self.handler.get_room_hierarchy(self.user, self.space)
+            self.handler.get_room_hierarchy(create_requester(self.user), self.space)
         )
 
         # Complete the two calls.
@@ -340,7 +354,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
 
         # A subsequent `get_room_hierarchy` call should not reuse the result.
         result3 = self.get_success(
-            self.handler.get_room_hierarchy(self.user, self.space)
+            self.handler.get_room_hierarchy(create_requester(self.user), self.space)
         )
         self._assert_hierarchy(result3, expected)
         self.assertIsNot(result1, result3)
@@ -359,9 +373,11 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
 
         # Run two `get_room_hierarchy` calls for different users up until they block.
         deferred1 = ensureDeferred(
-            self.handler.get_room_hierarchy(self.user, self.space)
+            self.handler.get_room_hierarchy(create_requester(self.user), self.space)
+        )
+        deferred2 = ensureDeferred(
+            self.handler.get_room_hierarchy(create_requester(user2), self.space)
         )
-        deferred2 = ensureDeferred(self.handler.get_room_hierarchy(user2, self.space))
 
         # Complete the two calls.
         result1 = self.get_success(deferred1)
@@ -465,7 +481,9 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         ]
         self._assert_rooms(result, expected)
 
-        result = self.get_success(self.handler.get_room_hierarchy(user2, self.space))
+        result = self.get_success(
+            self.handler.get_room_hierarchy(create_requester(user2), self.space)
+        )
         self._assert_hierarchy(result, expected)
 
     def test_complex_space(self):
@@ -507,7 +525,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         self._assert_rooms(result, expected)
 
         result = self.get_success(
-            self.handler.get_room_hierarchy(self.user, self.space)
+            self.handler.get_room_hierarchy(create_requester(self.user), self.space)
         )
         self._assert_hierarchy(result, expected)
 
@@ -522,7 +540,9 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         room_ids.append(self.room)
 
         result = self.get_success(
-            self.handler.get_room_hierarchy(self.user, self.space, limit=7)
+            self.handler.get_room_hierarchy(
+                create_requester(self.user), self.space, limit=7
+            )
         )
         # The result should have the space and all of the links, plus some of the
         # rooms and a pagination token.
@@ -534,7 +554,10 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         # Check the next page.
         result = self.get_success(
             self.handler.get_room_hierarchy(
-                self.user, self.space, limit=5, from_token=result["next_batch"]
+                create_requester(self.user),
+                self.space,
+                limit=5,
+                from_token=result["next_batch"],
             )
         )
         # The result should have the space and the room in it, along with a link
@@ -554,20 +577,22 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         room_ids.append(self.room)
 
         result = self.get_success(
-            self.handler.get_room_hierarchy(self.user, self.space, limit=7)
+            self.handler.get_room_hierarchy(
+                create_requester(self.user), self.space, limit=7
+            )
         )
         self.assertIn("next_batch", result)
 
         # Changing the room ID, suggested-only, or max-depth causes an error.
         self.get_failure(
             self.handler.get_room_hierarchy(
-                self.user, self.room, from_token=result["next_batch"]
+                create_requester(self.user), self.room, from_token=result["next_batch"]
             ),
             SynapseError,
         )
         self.get_failure(
             self.handler.get_room_hierarchy(
-                self.user,
+                create_requester(self.user),
                 self.space,
                 suggested_only=True,
                 from_token=result["next_batch"],
@@ -576,14 +601,19 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         )
         self.get_failure(
             self.handler.get_room_hierarchy(
-                self.user, self.space, max_depth=0, from_token=result["next_batch"]
+                create_requester(self.user),
+                self.space,
+                max_depth=0,
+                from_token=result["next_batch"],
             ),
             SynapseError,
         )
 
         # An invalid token is ignored.
         self.get_failure(
-            self.handler.get_room_hierarchy(self.user, self.space, from_token="foo"),
+            self.handler.get_room_hierarchy(
+                create_requester(self.user), self.space, from_token="foo"
+            ),
             SynapseError,
         )
 
@@ -609,14 +639,18 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
 
         # Test just the space itself.
         result = self.get_success(
-            self.handler.get_room_hierarchy(self.user, self.space, max_depth=0)
+            self.handler.get_room_hierarchy(
+                create_requester(self.user), self.space, max_depth=0
+            )
         )
         expected: List[Tuple[str, Iterable[str]]] = [(spaces[0], [rooms[0], spaces[1]])]
         self._assert_hierarchy(result, expected)
 
         # A single additional layer.
         result = self.get_success(
-            self.handler.get_room_hierarchy(self.user, self.space, max_depth=1)
+            self.handler.get_room_hierarchy(
+                create_requester(self.user), self.space, max_depth=1
+            )
         )
         expected += [
             (rooms[0], ()),
@@ -626,7 +660,9 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
 
         # A few layers.
         result = self.get_success(
-            self.handler.get_room_hierarchy(self.user, self.space, max_depth=3)
+            self.handler.get_room_hierarchy(
+                create_requester(self.user), self.space, max_depth=3
+            )
         )
         expected += [
             (rooms[1], ()),
@@ -657,7 +693,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
         self._assert_rooms(result, expected)
 
         result = self.get_success(
-            self.handler.get_room_hierarchy(self.user, self.space)
+            self.handler.get_room_hierarchy(create_requester(self.user), self.space)
         )
         self._assert_hierarchy(result, expected)
 
@@ -739,7 +775,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
             new=summarize_remote_room_hierarchy,
         ):
             result = self.get_success(
-                self.handler.get_room_hierarchy(self.user, self.space)
+                self.handler.get_room_hierarchy(create_requester(self.user), self.space)
             )
         self._assert_hierarchy(result, expected)
 
@@ -906,7 +942,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
             new=summarize_remote_room_hierarchy,
         ):
             result = self.get_success(
-                self.handler.get_room_hierarchy(self.user, self.space)
+                self.handler.get_room_hierarchy(create_requester(self.user), self.space)
             )
         self._assert_hierarchy(result, expected)
 
@@ -964,7 +1000,7 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
             new=summarize_remote_room_hierarchy,
         ):
             result = self.get_success(
-                self.handler.get_room_hierarchy(self.user, self.space)
+                self.handler.get_room_hierarchy(create_requester(self.user), self.space)
             )
         self._assert_hierarchy(result, expected)