From 2a55ad3956b9450ce910a44927fabe37f8246445 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 12 Apr 2024 11:19:00 +0100 Subject: [PATCH 1/4] Write a test to verify the invalid behaviour This test indeed fails due to the endpoint returning extra rooms. `_create_destination_rooms` was updated to allow specifying the `destination` that would be associated with the newly-created rooms. --- tests/rest/admin/test_federation.py | 65 +++++++++++++++++++++++++++-- 1 file changed, 62 insertions(+), 3 deletions(-) diff --git a/tests/rest/admin/test_federation.py b/tests/rest/admin/test_federation.py index c1d88f01765..0bec2861879 100644 --- a/tests/rest/admin/test_federation.py +++ b/tests/rest/admin/test_federation.py @@ -778,20 +778,79 @@ def test_destination_rooms(self) -> None: self.assertEqual(number_rooms, len(channel.json_body["rooms"])) self._check_fields(channel.json_body["rooms"]) - def _create_destination_rooms(self, number_rooms: int) -> None: - """Create a number rooms for destination + def test_room_filtering(self) -> None: + """Tests that rooms are correctly filtered""" + + # Create two rooms on the homeserver. Each has a different remote homeserver + # participating in it. + other_destination = "other.destination.org" + room_ids_self_dest = self._create_destination_rooms(2, destination=self.dest) + room_ids_other_dest = self._create_destination_rooms( + 1, destination=other_destination + ) + + # Ask for the rooms that `self.dest` is participating in. + channel = self.make_request("GET", self.url, access_token=self.admin_user_tok) + self.assertEqual(200, channel.code, msg=channel.json_body) + + # Verify that we received only the rooms that `self.dest` is participating in. + self.assertListEqual( + [r["room_id"] for r in channel.json_body["rooms"]], room_ids_self_dest + ) + self.assertEqual(channel.json_body["total"], len(room_ids_self_dest)) + + # Ask for the rooms that `other_destination` is participating in. + channel = self.make_request( + "GET", + self.url.replace(self.dest, other_destination), + access_token=self.admin_user_tok, + ) + self.assertEqual(200, channel.code, msg=channel.json_body) + + # Verify that we received only the rooms that `other_destination` is + # participating in. + self.assertListEqual( + [r["room_id"] for r in channel.json_body["rooms"]], room_ids_other_dest + ) + self.assertEqual(channel.json_body["total"], len(room_ids_other_dest)) + + def _create_destination_rooms( + self, + number_rooms: int, + destination: Optional[str] = None, + ) -> List[str]: + """ + Create the given number of rooms. The given `destination` homeserver will + be recorded as a participant. Args: number_rooms: Number of rooms to be created + destination: The domain of the homeserver that will be considered + as a participant in the rooms. + + Returns: + The IDs of the rooms that have been created. """ + room_ids = [] + + # If no destination was provided, default to `self.dest`. + if destination is None: + destination = self.dest + for _ in range(number_rooms): room_id = self.helper.create_room_as( self.admin_user, tok=self.admin_user_tok ) + room_ids.append(room_id) + self.get_success( - self.store.store_destination_rooms_entries((self.dest,), room_id, 1234) + self.store.store_destination_rooms_entries( + (destination,), room_id, 1234 + ) ) + return room_ids + def _check_fields(self, content: List[JsonDict]) -> None: """Checks that the expected room attributes are present in content From 1785ed9597cfdf7ce2ac52e71288e875b7ee0166 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 12 Apr 2024 11:28:27 +0100 Subject: [PATCH 2/4] Filter rooms by destination. Seems this was just missed in the original implementation. --- synapse/storage/databases/main/transactions.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/storage/databases/main/transactions.py b/synapse/storage/databases/main/transactions.py index 08e0241f683..770802483c7 100644 --- a/synapse/storage/databases/main/transactions.py +++ b/synapse/storage/databases/main/transactions.py @@ -660,6 +660,7 @@ def get_destination_rooms_paginate_txn( limit=limit, retcols=("room_id", "stream_ordering"), order_direction=order, + keyvalues={"destination": destination}, ), ) return rooms, count From a45935a928e9d74ea186400d7a151eabc367536b Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 12 Apr 2024 11:35:23 +0100 Subject: [PATCH 3/4] changelog --- changelog.d/17077.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/17077.bugfix diff --git a/changelog.d/17077.bugfix b/changelog.d/17077.bugfix new file mode 100644 index 00000000000..7d8ea374063 --- /dev/null +++ b/changelog.d/17077.bugfix @@ -0,0 +1 @@ +Fixes a bug introduced in v1.52.0 where the `destination` query parameter for the [Destination Rooms Admin API](https://element-hq.github.io/synapse/v1.105/usage/administration/admin_api/federation.html#destination-rooms) failed to actually filter returned rooms. \ No newline at end of file From e84feefa61abc10fb8e50229525da0592aa41794 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 15 Apr 2024 14:17:49 +0100 Subject: [PATCH 4/4] De-flake test Passing of the test was dependent on the order of the items in the returned lists. But no longer! --- tests/rest/admin/test_federation.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/rest/admin/test_federation.py b/tests/rest/admin/test_federation.py index 0bec2861879..c2015774a13 100644 --- a/tests/rest/admin/test_federation.py +++ b/tests/rest/admin/test_federation.py @@ -794,7 +794,9 @@ def test_room_filtering(self) -> None: self.assertEqual(200, channel.code, msg=channel.json_body) # Verify that we received only the rooms that `self.dest` is participating in. - self.assertListEqual( + # This assertion method name is a bit misleading. It does check that both lists + # contain the same items, and the same counts. + self.assertCountEqual( [r["room_id"] for r in channel.json_body["rooms"]], room_ids_self_dest ) self.assertEqual(channel.json_body["total"], len(room_ids_self_dest)) @@ -809,7 +811,7 @@ def test_room_filtering(self) -> None: # Verify that we received only the rooms that `other_destination` is # participating in. - self.assertListEqual( + self.assertCountEqual( [r["room_id"] for r in channel.json_body["rooms"]], room_ids_other_dest ) self.assertEqual(channel.json_body["total"], len(room_ids_other_dest))