mirror of
https://mau.dev/maunium/synapse.git
synced 2024-10-01 01:36:05 -04:00
Fix a bug that corrupted the cache of federated space hierarchies (#11775)
`FederationClient.get_room_hierarchy()` caches its return values, so refactor the code to avoid modifying the returned room summary.
This commit is contained in:
parent
5572e6cc4b
commit
af13a3be29
1
changelog.d/11775.bugfix
Normal file
1
changelog.d/11775.bugfix
Normal file
@ -0,0 +1 @@
|
||||
Fix a long-standing bug where space hierarchy over federation would only work correctly some of the time.
|
@ -118,7 +118,8 @@ class FederationClient(FederationBase):
|
||||
# It is a map of (room ID, suggested-only) -> the response of
|
||||
# get_room_hierarchy.
|
||||
self._get_room_hierarchy_cache: ExpiringCache[
|
||||
Tuple[str, bool], Tuple[JsonDict, Sequence[JsonDict], Sequence[str]]
|
||||
Tuple[str, bool],
|
||||
Tuple[JsonDict, Sequence[JsonDict], Sequence[JsonDict], Sequence[str]],
|
||||
] = ExpiringCache(
|
||||
cache_name="get_room_hierarchy_cache",
|
||||
clock=self._clock,
|
||||
@ -1333,7 +1334,7 @@ class FederationClient(FederationBase):
|
||||
destinations: Iterable[str],
|
||||
room_id: str,
|
||||
suggested_only: bool,
|
||||
) -> Tuple[JsonDict, Sequence[JsonDict], Sequence[str]]:
|
||||
) -> Tuple[JsonDict, Sequence[JsonDict], Sequence[JsonDict], Sequence[str]]:
|
||||
"""
|
||||
Call other servers to get a hierarchy of the given room.
|
||||
|
||||
@ -1348,7 +1349,8 @@ class FederationClient(FederationBase):
|
||||
|
||||
Returns:
|
||||
A tuple of:
|
||||
The room as a JSON dictionary.
|
||||
The room as a JSON dictionary, without a "children_state" key.
|
||||
A list of `m.space.child` state events.
|
||||
A list of children rooms, as JSON dictionaries.
|
||||
A list of inaccessible children room IDs.
|
||||
|
||||
@ -1363,7 +1365,7 @@ class FederationClient(FederationBase):
|
||||
|
||||
async def send_request(
|
||||
destination: str,
|
||||
) -> Tuple[JsonDict, Sequence[JsonDict], Sequence[str]]:
|
||||
) -> Tuple[JsonDict, Sequence[JsonDict], Sequence[JsonDict], Sequence[str]]:
|
||||
try:
|
||||
res = await self.transport_layer.get_room_hierarchy(
|
||||
destination=destination,
|
||||
@ -1392,7 +1394,7 @@ class FederationClient(FederationBase):
|
||||
raise InvalidResponseError("'room' must be a dict")
|
||||
|
||||
# Validate children_state of the room.
|
||||
children_state = room.get("children_state", [])
|
||||
children_state = room.pop("children_state", [])
|
||||
if not isinstance(children_state, Sequence):
|
||||
raise InvalidResponseError("'room.children_state' must be a list")
|
||||
if any(not isinstance(e, dict) for e in children_state):
|
||||
@ -1421,7 +1423,7 @@ class FederationClient(FederationBase):
|
||||
"Invalid room ID in 'inaccessible_children' list"
|
||||
)
|
||||
|
||||
return room, children, inaccessible_children
|
||||
return room, children_state, children, inaccessible_children
|
||||
|
||||
try:
|
||||
result = await self._try_destination_list(
|
||||
@ -1469,8 +1471,6 @@ class FederationClient(FederationBase):
|
||||
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 = []
|
||||
@ -1480,7 +1480,7 @@ class FederationClient(FederationBase):
|
||||
|
||||
# It isn't clear from the response whether some of the rooms are
|
||||
# not accessible.
|
||||
result = (requested_room, children, ())
|
||||
result = (requested_room, children_events, children, ())
|
||||
|
||||
# Cache the result to avoid fetching data over federation every time.
|
||||
self._get_room_hierarchy_cache[(room_id, suggested_only)] = result
|
||||
|
@ -780,6 +780,7 @@ class RoomSummaryHandler:
|
||||
try:
|
||||
(
|
||||
room_response,
|
||||
children_state_events,
|
||||
children,
|
||||
inaccessible_children,
|
||||
) = await self._federation_client.get_room_hierarchy(
|
||||
@ -804,7 +805,7 @@ class RoomSummaryHandler:
|
||||
}
|
||||
|
||||
return (
|
||||
_RoomEntry(room_id, room_response, room_response.pop("children_state", ())),
|
||||
_RoomEntry(room_id, room_response, children_state_events),
|
||||
children_by_room_id,
|
||||
set(inaccessible_children),
|
||||
)
|
||||
|
@ -28,6 +28,7 @@ from synapse.api.constants import (
|
||||
from synapse.api.errors import AuthError, NotFoundError, SynapseError
|
||||
from synapse.api.room_versions import RoomVersions
|
||||
from synapse.events import make_event_from_dict
|
||||
from synapse.federation.transport.client import TransportLayerClient
|
||||
from synapse.handlers.room_summary import _child_events_comparison_key, _RoomEntry
|
||||
from synapse.rest import admin
|
||||
from synapse.rest.client import login, room
|
||||
@ -134,10 +135,18 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
|
||||
self._add_child(self.space, self.room, self.token)
|
||||
|
||||
def _add_child(
|
||||
self, space_id: str, room_id: str, token: str, order: Optional[str] = None
|
||||
self,
|
||||
space_id: str,
|
||||
room_id: str,
|
||||
token: str,
|
||||
order: Optional[str] = None,
|
||||
via: Optional[List[str]] = None,
|
||||
) -> None:
|
||||
"""Add a child room to a space."""
|
||||
content: JsonDict = {"via": [self.hs.hostname]}
|
||||
if via is None:
|
||||
via = [self.hs.hostname]
|
||||
|
||||
content: JsonDict = {"via": via}
|
||||
if order is not None:
|
||||
content["order"] = order
|
||||
self.helper.send_state(
|
||||
@ -1036,6 +1045,85 @@ class SpaceSummaryTestCase(unittest.HomeserverTestCase):
|
||||
)
|
||||
self._assert_hierarchy(result, expected)
|
||||
|
||||
def test_fed_caching(self):
|
||||
"""
|
||||
Federation `/hierarchy` responses should be cached.
|
||||
"""
|
||||
fed_hostname = self.hs.hostname + "2"
|
||||
fed_subspace = "#space:" + fed_hostname
|
||||
fed_room = "#room:" + fed_hostname
|
||||
|
||||
# Add a room to the space which is on another server.
|
||||
self._add_child(self.space, fed_subspace, self.token, via=[fed_hostname])
|
||||
|
||||
federation_requests = 0
|
||||
|
||||
async def get_room_hierarchy(
|
||||
_self: TransportLayerClient,
|
||||
destination: str,
|
||||
room_id: str,
|
||||
suggested_only: bool,
|
||||
) -> JsonDict:
|
||||
nonlocal federation_requests
|
||||
federation_requests += 1
|
||||
|
||||
return {
|
||||
"room": {
|
||||
"room_id": fed_subspace,
|
||||
"world_readable": True,
|
||||
"room_type": RoomTypes.SPACE,
|
||||
"children_state": [
|
||||
{
|
||||
"type": EventTypes.SpaceChild,
|
||||
"room_id": fed_subspace,
|
||||
"state_key": fed_room,
|
||||
"content": {"via": [fed_hostname]},
|
||||
},
|
||||
],
|
||||
},
|
||||
"children": [
|
||||
{
|
||||
"room_id": fed_room,
|
||||
"world_readable": True,
|
||||
},
|
||||
],
|
||||
"inaccessible_children": [],
|
||||
}
|
||||
|
||||
expected = [
|
||||
(self.space, [self.room, fed_subspace]),
|
||||
(self.room, ()),
|
||||
(fed_subspace, [fed_room]),
|
||||
(fed_room, ()),
|
||||
]
|
||||
|
||||
with mock.patch(
|
||||
"synapse.federation.transport.client.TransportLayerClient.get_room_hierarchy",
|
||||
new=get_room_hierarchy,
|
||||
):
|
||||
result = self.get_success(
|
||||
self.handler.get_room_hierarchy(create_requester(self.user), self.space)
|
||||
)
|
||||
self.assertEqual(federation_requests, 1)
|
||||
self._assert_hierarchy(result, expected)
|
||||
|
||||
# The previous federation response should be reused.
|
||||
result = self.get_success(
|
||||
self.handler.get_room_hierarchy(create_requester(self.user), self.space)
|
||||
)
|
||||
self.assertEqual(federation_requests, 1)
|
||||
self._assert_hierarchy(result, expected)
|
||||
|
||||
# Expire the response cache
|
||||
self.reactor.advance(5 * 60 + 1)
|
||||
|
||||
# A new federation request should be made.
|
||||
result = self.get_success(
|
||||
self.handler.get_room_hierarchy(create_requester(self.user), self.space)
|
||||
)
|
||||
self.assertEqual(federation_requests, 2)
|
||||
self._assert_hierarchy(result, expected)
|
||||
|
||||
|
||||
class RoomSummaryTestCase(unittest.HomeserverTestCase):
|
||||
servlets = [
|
||||
|
Loading…
Reference in New Issue
Block a user