From 4f685c6c4031807f14028dc3696a497b4d5e9844 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Sun, 5 Jan 2020 00:15:14 -0500 Subject: [PATCH 01/22] Admin API to list, filter and sort rooms --- synapse/handlers/admin.py | 26 ++ synapse/handlers/directory.py | 2 +- synapse/rest/admin/__init__.py | 3 +- synapse/rest/admin/rooms.py | 53 ++++ synapse/storage/data_stores/main/room.py | 162 ++++++++++- tests/rest/admin/test_admin.py | 338 ++++++++++++++++++++++- 6 files changed, 580 insertions(+), 4 deletions(-) diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index 1a4ba1238588..bdb382d09fc7 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -14,6 +14,7 @@ # limitations under the License. import logging +from typing import Optional from synapse.api.constants import Membership from synapse.types import RoomStreamToken @@ -81,6 +82,31 @@ async def get_users_paginate(self, start, limit, name, guests, deactivated): return ret + async def get_rooms_paginate( + self, + start: int, + limit: int, + order_by: str, + reverse_order: bool, + search_term: Optional[str], + ): + """Function to retrieve a paginated list of rooms as json. + + Args: + start: offset in the list + limit: maximum amount of rooms to retrieve + order_by: the sort order of the returned list. valid values: + * alphabetical (default) + * size + reverse_order: whether to reverse the room list + search_term: a string to filter room names by + Returns: + defer.Deferred: a json list[dict[str, Any]] + """ + return await self.store.get_rooms_paginate( + start, limit, order_by, reverse_order, search_term + ) + async def search_users(self, term): """Function to search users list for one or more users with the matched term. diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index a07d2f1a17e8..5a082fdc6a40 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -263,7 +263,7 @@ def get_association(self, room_alias): if not room_id: raise SynapseError( 404, - "Room alias %s not found" % (room_alias.to_string(),), + "Room alias %s not found!" % (room_alias.to_string(),), Codes.NOT_FOUND, ) diff --git a/synapse/rest/admin/__init__.py b/synapse/rest/admin/__init__.py index c122c449f401..258b0a1994a3 100644 --- a/synapse/rest/admin/__init__.py +++ b/synapse/rest/admin/__init__.py @@ -29,7 +29,7 @@ from synapse.rest.admin.groups import DeleteGroupAdminRestServlet from synapse.rest.admin.media import ListMediaInRoom, register_servlets_for_media_repo from synapse.rest.admin.purge_room_servlet import PurgeRoomServlet -from synapse.rest.admin.rooms import ShutdownRoomRestServlet +from synapse.rest.admin.rooms import ListRoomRestServlet, ShutdownRoomRestServlet from synapse.rest.admin.server_notice_servlet import SendServerNoticeServlet from synapse.rest.admin.users import ( AccountValidityRenewServlet, @@ -187,6 +187,7 @@ def register_servlets(hs, http_server): Register all the admin servlets. """ register_servlets_for_client_rest_resource(hs, http_server) + ListRoomRestServlet(hs).register(http_server) PurgeRoomServlet(hs).register(http_server) SendServerNoticeServlet(hs).register(http_server) VersionServlet(hs).register(http_server) diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index f7cc5e9be9cb..6bc6699ecb57 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -15,10 +15,13 @@ import logging from synapse.api.constants import Membership +from synapse.api.errors import Codes, SynapseError from synapse.http.servlet import ( RestServlet, assert_params_in_dict, + parse_integer, parse_json_object_from_request, + parse_string, ) from synapse.rest.admin._base import ( assert_user_is_admin, @@ -155,3 +158,53 @@ async def on_POST(self, request, room_id): "new_room_id": new_room_id, }, ) + + +class ListRoomRestServlet(RestServlet): + """ + List all rooms that are known to the homeserver. Results are returned + in a dictionary containing room information. Supports pagination. + """ + + PATTERNS = historical_admin_path_patterns("/rooms") + + def __init__(self, hs): + self.hs = hs + self.store = hs.get_datastore() + self.auth = hs.get_auth() + self.admin_handler = hs.get_handlers().admin_handler + + async def on_GET(self, request): + requester = await self.auth.get_user_by_req(request) + await assert_user_is_admin(self.auth, requester.user) + + # Extract query parameters + start = parse_integer(request, "from", default=0) + limit = parse_integer(request, "limit", default=100) + order_by = parse_string(request, "order_by", default="alphabetical") + search_term = parse_string(request, "search_term") + + direction = parse_string(request, "dir", default="f") + if direction != "f" and direction != "b": + raise SynapseError( + 400, "Unknown direction: %s" % direction, errcode=Codes.INVALID_PARAM + ) + reverse_order = True if direction == "b" else False + + if search_term == "": + raise SynapseError( + 400, + "search_term cannot be an empty string", + errcode=Codes.INVALID_PARAM, + ) + + # Return list of rooms according to parameters + rooms, next_token = await self.admin_handler.get_rooms_paginate( + start, limit, order_by, reverse_order, search_term + ) + response = {"rooms": rooms} + + if next_token: + response["next_token"] = next_token + + return 200, response diff --git a/synapse/storage/data_stores/main/room.py b/synapse/storage/data_stores/main/room.py index aa476d0fbf0b..2895ef07006d 100644 --- a/synapse/storage/data_stores/main/room.py +++ b/synapse/storage/data_stores/main/room.py @@ -17,7 +17,7 @@ import collections import logging import re -from typing import Optional, Tuple +from typing import List, Optional, Tuple from six import integer_types @@ -280,6 +280,123 @@ def is_room_blocked(self, room_id): desc="is_room_blocked", ) + async def get_rooms_paginate( + self, + start: int, + limit: int, + order_by: str, + reverse_order: bool, + search_term: Optional[str], + ): + """Function to retrieve a paginated list of rooms as json. + + Args: + start: offset in the list + limit: maximum amount of rooms to retrieve + order_by: the sort order of the returned list. valid values: + * alphabetical (default) + * size + reverse_order: whether to reverse the room list + search_term: a string to filter room names by + Returns: + defer.Deferred: a json list[dict[str, Any]] + """ + # Filter room names by a string + where_statement = "" + if search_term: + where_statement = "WHERE state.name LIKE '%' || ? || '%'" + + # Set ordering + if order_by == "size": + order_by_column = "curr.joined_members" + order_by_asc = False + else: + # Sort alphabetically + order_by_column = "state.name" + order_by_asc = True + + # Whether to return the list in reverse order + if reverse_order: + # Flip the boolean + order_by_asc = not order_by_asc + + # Create one query for getting the limited number of events that the user asked + # for, and another query for getting the total number of events that could be + # returned. Thus allowing us to see if there are more events to paginate through + info_sql = """ + SELECT state.room_id, state.name, state.canonical_alias, curr.joined_members + FROM room_stats_state state + LEFT JOIN room_stats_current curr ON state.room_id = curr.room_id + %s + ORDER BY %s %s + LIMIT ? + OFFSET ? + """ % ( + where_statement, + order_by_column, + "ASC" if order_by_asc else "DESC", + ) + + # Use a nested SELECT statement as SQL can't count(*) with an OFFSET + count_sql = """ + SELECT count(*) FROM ( + SELECT room_id FROM room_stats_state state + %s + LIMIT ? + OFFSET ? + ) + """ % ( + where_statement, + ) + + def _get_rooms_paginate_txn(txn): + # Execute the data query + sql_values = (limit, start) + if search_term: + # Add the search term into the WHERE clause + sql_values = (search_term,) + sql_values + txn.execute(info_sql, sql_values) + rows = [row for row in txn] + + # Execute the count query + sql_values = (-1, start) # Set LIMIT to -1. OFFSET can't be used on its own + if search_term: + # Add the search term into the WHERE clause + sql_values = (search_term,) + sql_values + txn.execute(count_sql, sql_values) + room_count = txn.fetchone() + if room_count: + return rows, room_count[0] + + # room_count can be None if there are no rows and OFFSET > 1 + # Set to 0 instead + return rows, 0 + + room_tuples, total_rooms = await self.db.runInteraction( + "get_rooms_paginate", _get_rooms_paginate_txn, + ) + rooms = [] + + # Refactor rooms into structured dictionary + for room in room_tuples: + rooms.append( + { + "room_id": room[0], + "name": room[1], + "canonical_alias": room[2], + "joined_members": room[3], + } + ) + + # Are there more rooms to paginate through after this? + next_token = None + if total_rooms > len(room_tuples): + # There are. Calculate where the query should start from next time to get + # the next part of the list + next_token = start + limit + + return rooms, next_token + @cachedInlineCallbacks(max_entries=10000) def get_ratelimit_for_user(self, user_id): """Check if there are any overrides for ratelimiting for the given @@ -721,6 +838,49 @@ def get_all_new_public_rooms(txn): "get_all_new_public_rooms", get_all_new_public_rooms ) + @defer.inlineCallbacks + def get_all_rooms_with_alias( + self, offset: int, limit: int + ) -> List[Tuple[str, str, str]]: + """Retrieves a list of all rooms the server is in as well as all room + aliases for that room. Rooms are retrieved in alphabetical order by room ID. + + Args: + offset: What to offset the list of rooms by + limit: The maximum number of rooms to return + + Returns: + A list of tuples containing the (room_id, room_name, main room_alias) + """ + + def get_all_rooms_with_alias_txn(txn): + # Retrieve all known room_ids and aliases + sql = """ + SELECT rooms.room_id, room_aliases.room_alias + FROM rooms + INNER JOIN room_aliases on room_aliases.room_id = rooms.room_id + ORDER BY rooms.room_id ASC + LIMIT ? + OFFSET ? + """ + + txn.execute(sql, (limit, offset)) + room_ids_and_aliases = txn.fetchall() + + # Extract all known aliases for a room + room_id_to_alias_list = {} + for room_id, room_alias in room_ids_and_aliases: + if room_id in room_id_to_alias_list: + room_id_to_alias_list[room_id].append(room_alias) + else: + room_id_to_alias_list[room_id] = [room_alias] + + return room_id_to_alias_list + + return self.db.runInteraction( + "get_all_rooms_with_alias", get_all_rooms_with_alias_txn + ) + @defer.inlineCallbacks def block_room(self, room_id, user_id): """Marks the room as blocked. Can be called multiple times. diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index 0ed259438134..64c500ef9f6b 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -16,6 +16,8 @@ import hashlib import hmac import json +import urllib.parse +from typing import List, Optional from mock import Mock @@ -23,7 +25,7 @@ from synapse.api.constants import UserTypes from synapse.http.server import JsonResource from synapse.rest.admin import VersionServlet -from synapse.rest.client.v1 import events, login, room +from synapse.rest.client.v1 import directory, events, login, room from synapse.rest.client.v2_alpha import groups from tests import unittest @@ -643,3 +645,337 @@ def test_purge_room(self): self.assertEqual(count, 0, msg="Rows not purged in {}".format(table)) test_purge_room.skip = "Disabled because it's currently broken" + + +class RoomTestCase(unittest.HomeserverTestCase): + """Test /room admin API. + """ + + servlets = [ + synapse.rest.admin.register_servlets, + login.register_servlets, + room.register_servlets, + directory.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.store = hs.get_datastore() + + # Create user + self.admin_user = self.register_user("user5", "pass", admin=True) + self.admin_user_tok = self.login("user5", "pass") + + def test_list_rooms(self): + """Test that we can list rooms""" + # Create 3 test rooms + total_rooms = 3 + room_ids = [] + for x in range(total_rooms): + room_id = self.helper.create_room_as( + self.admin_user, tok=self.admin_user_tok + ) + room_ids.append(room_id) + + # Request the list of rooms + url = "/_synapse/admin/v1/rooms" + request, channel = self.make_request( + "GET", url.encode("ascii"), access_token=self.admin_user_tok, + ) + self.render(request) + + # Check request completed successfully + self.assertEqual(200, int(channel.code), msg=channel.json_body) + + # Check that response json body contains a "rooms" key + self.assertTrue( + "rooms" in channel.json_body, + msg="Response body does not " "contain a 'rooms' key", + ) + + # Check that 3 rooms were returned + self.assertEqual(3, len(channel.json_body["rooms"]), msg=channel.json_body) + + # Check their room_ids match + returned_room_ids = [room["room_id"] for room in channel.json_body["rooms"]] + common_room_ids = set(room_ids) & set(returned_room_ids) + self.assertEqual( + len(common_room_ids), + total_rooms, + msg="Different room_ids than expected returned. " + "Expected:\n%s\nReturned:\n%s" % (room_ids, returned_room_ids), + ) + + # Check that all fields are available + for r in channel.json_body["rooms"]: + self.assertIn("name", r) + self.assertIn("canonical_alias", r) + self.assertIn("joined_members", r) + + # We shouldn't receive a next token here as there's no further rooms to show + self.assertTrue("next_token" not in channel.json_body) + + @unittest.DEBUG + def test_list_rooms_pagination(self): + """Test that we can get a full list of rooms through pagination""" + # Create 5 test rooms + total_rooms = 5 + room_ids = [] + for x in range(total_rooms): + room_id = self.helper.create_room_as( + self.admin_user, tok=self.admin_user_tok + ) + room_ids.append(room_id) + + returned_room_ids = [] + + # Request the list of rooms + start = 0 + limit = 2 + + run_count = 0 + should_repeat = True + while should_repeat: + run_count += 1 + + url = "/_synapse/admin/v1/rooms?from=%d&limit=%d" % (start, limit) + request, channel = self.make_request( + "GET", url.encode("ascii"), access_token=self.admin_user_tok, + ) + self.render(request) + self.assertEqual( + 200, int(channel.result["code"]), msg=channel.result["body"] + ) + + self.assertTrue("rooms" in channel.json_body) + for r in channel.json_body["rooms"]: + returned_room_ids.append(r["room_id"]) + + if "next_token" not in channel.json_body: + # We have reached the end of the list + should_repeat = False + else: + # Make another query with an updated start value + start = channel.json_body["next_token"] + + # We should've queried the endpoint 3 times + self.assertEqual( + run_count, + 3, + msg="Should've queried 3 times for 5 rooms with " "limit 2 per query", + ) + + # Check that we received all of the room ids + common_room_ids = set(room_ids) & set(returned_room_ids) + self.assertEqual( + len(common_room_ids), + total_rooms, + msg="Different room_ids than expected returned. " + "Expected:\n%s\nReturned:\n%s" % (room_ids, returned_room_ids), + ) + + url = "/_synapse/admin/v1/rooms?from=%d&limit=%d" % (start, limit) + request, channel = self.make_request( + "GET", url.encode("ascii"), access_token=self.admin_user_tok, + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + def test_correct_room_attributes(self): + """Test the correct attributes for a room are returned""" + # Create a test room + room_id = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok) + + test_alias = "#test:test" + test_room_name = "something" + + # Have another user join the room + user_2 = self.register_user("user4", "pass") + user_tok_2 = self.login("user4", "pass") + self.helper.join(room_id, user_2, tok=user_tok_2) + + # Create a new alias to this room + url = "/_matrix/client/r0/directory/room/%s" % (urllib.parse.quote(test_alias),) + request, channel = self.make_request( + "PUT", + url.encode("ascii"), + {"room_id": room_id}, + access_token=self.admin_user_tok, + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # Set this new alias as the canonical alias for this room + self.helper.send_state( + room_id, + "m.room.aliases", + {"aliases": [test_alias]}, + tok=self.admin_user_tok, + state_key="test", + ) + self.helper.send_state( + room_id, + "m.room.canonical_alias", + {"alias": test_alias}, + tok=self.admin_user_tok, + ) + + # Set a name for the room + self.helper.send_state( + room_id, "m.room.name", {"name": test_room_name}, tok=self.admin_user_tok, + ) + + # Request the list of rooms + url = "/_synapse/admin/v1/rooms" + request, channel = self.make_request( + "GET", url.encode("ascii"), access_token=self.admin_user_tok, + ) + self.render(request) + self.assertEqual(200, int(channel.result["code"]), msg=channel.result["body"]) + + # Check that rooms were returned + self.assertTrue("rooms" in channel.json_body) + rooms = channel.json_body["rooms"] + + # Check that only one room was returned + self.assertEqual(len(rooms), 1) + + # Check that there is no `next_token` + self.assertNotIn("next_token", channel.json_body) + + # Check that all provided attributes are set + r = rooms[0] + self.assertEqual(room_id, r["room_id"]) + self.assertEqual(test_room_name, r["name"]) + self.assertEqual(test_alias, r["canonical_alias"]) + + def test_room_list_sort_order(self): + """Test room list sort ordering. alphabetical versus number of members, + reversing the order, etc. + """ + # Create 3 test rooms + room_id_1 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok) + room_id_2 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok) + room_id_3 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok) + + # Set room names in alphabetical order. room 1 -> A, 2 -> B, 3 -> C + self.helper.send_state( + room_id_1, "m.room.name", {"name": "A"}, tok=self.admin_user_tok, + ) + self.helper.send_state( + room_id_2, "m.room.name", {"name": "B"}, tok=self.admin_user_tok, + ) + self.helper.send_state( + room_id_3, "m.room.name", {"name": "C"}, tok=self.admin_user_tok, + ) + + # Set room member size in the reverse order. room 1 -> 1 member, 2 -> 2, 3 -> 3 + user_1 = self.register_user("bob1", "pass") + user_1_tok = self.login("bob1", "pass") + self.helper.join(room_id_2, user_1, tok=user_1_tok) + + user_2 = self.register_user("bob2", "pass") + user_2_tok = self.login("bob2", "pass") + self.helper.join(room_id_3, user_2, tok=user_2_tok) + + user_3 = self.register_user("bob3", "pass") + user_3_tok = self.login("bob3", "pass") + self.helper.join(room_id_3, user_3, tok=user_3_tok) + + def _order_test( + order_type: str, expected_room_list: List[str], reverse: bool = False, + ): + """Request the list of rooms in a certain order. Assert that order is what + we expect + + Args: + order_type: The type of ordering to give the server + expected_room_list: The list of room_ids in the order we expect to get + back from the server + """ + # Request the list of rooms in the given order + url = "/_synapse/admin/v1/rooms?order_by=%s" % (order_type,) + if reverse: + url += "&dir=b" + request, channel = self.make_request( + "GET", url.encode("ascii"), access_token=self.admin_user_tok, + ) + self.render(request) + self.assertEqual(200, channel.code, msg=channel.json_body) + + # Check that rooms were returned + self.assertTrue("rooms" in channel.json_body) + rooms = channel.json_body["rooms"] + + # Check that rooms were returned in alphabetical order + returned_order = [r["room_id"] for r in rooms] + self.assertListEqual(expected_room_list, returned_order) # order is checked + + # Test different sort orders, with forward and reverse directions + _order_test("alphabetical", [room_id_1, room_id_2, room_id_3]) + _order_test("alphabetical", [room_id_3, room_id_2, room_id_1], reverse=True) + + _order_test("size", [room_id_3, room_id_2, room_id_1]) + _order_test("size", [room_id_1, room_id_2, room_id_3], reverse=True) + + def test_search_term(self): + """Test that searching for a room works correctly""" + # Create two test rooms + room_id_1 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok) + room_id_2 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok) + + room_name_1 = "something" + room_name_2 = "else" + + # Set the name for each room + self.helper.send_state( + room_id_1, "m.room.name", {"name": room_name_1}, tok=self.admin_user_tok, + ) + self.helper.send_state( + room_id_2, "m.room.name", {"name": room_name_2}, tok=self.admin_user_tok, + ) + + def _search_test( + expected_room_id: Optional[str], + search_term: str, + expected_http_code: int = 200, + ): + """Search for a room and check that the returned room's id is a match + + Args: + expected_room_id: The room_id expected to be returned by the API. Set + to None to expect zero results for the search + search_term: The term to search for room names with + expected_http_code: The expected http code for the request + """ + url = "/_synapse/admin/v1/rooms?search_term=%s" % (search_term,) + request, channel = self.make_request( + "GET", url.encode("ascii"), access_token=self.admin_user_tok, + ) + self.render(request) + self.assertEqual(expected_http_code, channel.code, msg=channel.json_body) + + if expected_http_code != 200: + return + + # Check that rooms were returned + self.assertTrue("rooms" in channel.json_body) + rooms = channel.json_body["rooms"] + + # Check that the expected number of rooms were returned + self.assertEqual(len(rooms), 1 if expected_room_id else 0) + + if expected_room_id: + # Check that the first returned room id is correct + r = rooms[0] + self.assertEqual(expected_room_id, r["room_id"]) + + # Perform search tests + _search_test(room_id_1, "something") + _search_test(room_id_1, "thing") + + _search_test(room_id_2, "else") + _search_test(room_id_2, "se") + + _search_test(None, "foo") + _search_test(None, "bar") + _search_test(None, "", expected_http_code=400) From 908775cf33bfb4a20b6d77af9d527eafea773543 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Thu, 16 Jan 2020 17:44:21 +0000 Subject: [PATCH 02/22] Add changelog --- changelog.d/6720.feature | 1 + synapse/handlers/admin.py | 2 +- tests/rest/admin/test_admin.py | 7 ++----- 3 files changed, 4 insertions(+), 6 deletions(-) create mode 100644 changelog.d/6720.feature diff --git a/changelog.d/6720.feature b/changelog.d/6720.feature new file mode 100644 index 000000000000..dfc1b74d621a --- /dev/null +++ b/changelog.d/6720.feature @@ -0,0 +1 @@ +Add a new admin API to list and filter rooms on the server. \ No newline at end of file diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index 20b47a96e444..15e9696f2615 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -14,7 +14,7 @@ # limitations under the License. import logging -from typing import Optional, List +from typing import List, Optional from synapse.api.constants import Membership from synapse.events import FrozenEvent diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index 47e60411b7fc..f9e7c68bdd76 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -14,11 +14,10 @@ # limitations under the License. import json -import urllib.parse -from typing import List, Optional import os import urllib.parse from binascii import unhexlify +from typing import List, Optional from mock import Mock @@ -470,9 +469,7 @@ def test_quarantine_media_by_id(self): ) # Extract media ID from the response - server_name_and_media_id = response["content_uri"][ - 6: - ] # Cut off the 'mxc://' bit + server_name_and_media_id = response["content_uri"][6:] # Cut off 'mxc://' server_name, media_id = server_name_and_media_id.split("/") # Attempt to access the media From df322762778a82f1b92e6d91480c79cb803f9a31 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Jan 2020 11:03:48 +0000 Subject: [PATCH 03/22] Add alias to subquery for postgres --- docs/admin_api/rooms.md | 0 synapse/storage/data_stores/main/room.py | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 docs/admin_api/rooms.md diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md new file mode 100644 index 000000000000..e69de29bb2d1 diff --git a/synapse/storage/data_stores/main/room.py b/synapse/storage/data_stores/main/room.py index c38270f039a5..593414fb996f 100644 --- a/synapse/storage/data_stores/main/room.py +++ b/synapse/storage/data_stores/main/room.py @@ -345,7 +345,7 @@ async def get_rooms_paginate( %s LIMIT ? OFFSET ? - ) + ) AS get_room_ids """ % ( where_statement, ) From 5aa30d890b8ef5c793a24a507470df3d8dceb622 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Jan 2020 11:54:45 +0000 Subject: [PATCH 04/22] Add documentation --- docs/admin_api/rooms.md | 152 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 152 insertions(+) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index e69de29bb2d1..0a19bfaf4874 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -0,0 +1,152 @@ +# List Room API + +The List Room admin API allows server admins to get a list of rooms on their +server. There are various parameters available that allow for filtering and +sorting the returned list. This API supports pagination. + +## Parameters + +The following query parameters are available: + +* `from` - Offset in the returned list. Defaults to `0`. +* `limit` - Maximum amount of rooms to return. Defaults to `100`. +* `order_by` - The method in which to sort the returned list of rooms. Valid values are: + - `"alphabetical"` - Rooms are ordered alphabetically by room name. This is the default. + - `"size"` - Rooms are ordered by the number of members. Largest to smallest. +* `dir` - Direction of room order. Either `"f"` for forwards or `"b"` for backwards. Setting +this value to `"b"` will reverse the above sort order. Defaults to `"f"`. +* `search_term` - Filter rooms by their room name. Search term can be contained in any +part of the room name. Defaults to no filtering. + +The following fields are possible in the JSON response body: + +* `rooms` - An array of objects, each containing information about a room. + - Room objects contain the following fields: + - `room_id` - The ID of the room. + - `name` - The name of the room. + - `canonical_alias` - The canonical (main) alias address of the room. + - `joined_members` - How many users are currently in the room. +* `next_token` - If this field is present, we know that there are potentially +more rooms on the server that did not all fit into this response. We can use +`next_token` to get the "next page" of results. To do so, simply repeat your +request, setting the `from` parameter to the value of `next_token`. + +## Usage + +A standard request with no filtering: + +``` +GET /_synapse/admin/rooms + +{} +``` + +Response: + +``` +{ + "rooms": [ + { + "room_id": "!OGEhHVWSdvArJzumhm:matrix.org", + "name": "Matrix HQ", + "canonical_alias": "#matrix:matrix.org", + "joined_members": 8326 + }, + ... + { + "room_id": "!xYvNcQPhnkrdUmYczI:matrix.org", + "name": "This Week In Matrix (TWIM)", + "canonical_alias": "#twim:matrix.org", + "joined_members": 314 + } + ] +} +``` + +Filtering by room name: + +``` +GET /_synapse/admin/rooms?search_term=TWIM + +{} +``` + +Response: + +``` +{ + "rooms": [ + { + "room_id": "!xYvNcQPhnkrdUmYczI:matrix.org", + "name": "This Week In Matrix (TWIM)", + "canonical_alias": "#twim:matrix.org", + "joined_members": 314 + } + ] +} +``` + +Paginating through a list of rooms: + +``` +GET /_synapse/admin/rooms?order_by=size + +{} +``` + +Response: + +``` +{ + "rooms": [ + { + "room_id": "!OGEhHVWSdvArJzumhm:matrix.org", + "name": "Matrix HQ", + "canonical_alias": "#matrix:matrix.org", + "joined_members": 8326 + }, + ... (98 hidden items) ... + { + "room_id": "!xYvNcQPhnkrdUmYczI:matrix.org", + "name": "This Week In Matrix (TWIM)", + "canonical_alias": "#twim:matrix.org", + "joined_members": 314 + } + ], + "next_token": 100 +} +``` + +The presence of the `next_token` parameter tells us that there are more rooms that stated in +this request, and we need to make another request to get them. To get the next batch of room +results, we repeat our request, setting the `from` parameter to the value of `next_token`. + +``` +GET /_synapse/admin/rooms?order_by=size&from=100 + +{} +``` + +Response: + +``` +{ + "rooms": [ + { + "room_id": "!mscvqgqpHYjBGDxNym:matrix.org", + "name": "Music Theory", + "canonical_alias": "#musictheory:matrix.org", + "joined_members": 127 + }, + ... (65 hidden items) ... + { + "room_id": "!twcBhHVdZlQWuuxBhN:termina.org.uk", + "name": "weechat-matrix", + "canonical_alias": "#weechat-matrix:termina.org.uk", + "joined_members": 137 + } + ] +} +``` + +Once the `next_token` parameter is not present, we know we've reached the end of the list. From 5552f97ff0313454f084556b9c76e6b705871ac7 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Jan 2020 11:56:59 +0000 Subject: [PATCH 05/22] Remove potentially confusing quotation marks --- docs/admin_api/rooms.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 0a19bfaf4874..5408179f325d 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -13,8 +13,8 @@ The following query parameters are available: * `order_by` - The method in which to sort the returned list of rooms. Valid values are: - `"alphabetical"` - Rooms are ordered alphabetically by room name. This is the default. - `"size"` - Rooms are ordered by the number of members. Largest to smallest. -* `dir` - Direction of room order. Either `"f"` for forwards or `"b"` for backwards. Setting -this value to `"b"` will reverse the above sort order. Defaults to `"f"`. +* `dir` - Direction of room order. Either `"f"` for forwards or `b` for backwards. Setting +this value to `b` will reverse the above sort order. Defaults to `f`. * `search_term` - Filter rooms by their room name. Search term can be contained in any part of the room name. Defaults to no filtering. From a5a7e2e0389715e4966b1cf12b792f885bcb074e Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Jan 2020 12:15:22 +0000 Subject: [PATCH 06/22] Minor fixups --- docs/admin_api/rooms.md | 21 +++++---- synapse/handlers/admin.py | 5 ++- synapse/handlers/directory.py | 2 +- synapse/rest/admin/rooms.py | 19 ++++++--- synapse/storage/data_stores/main/room.py | 54 ++++-------------------- 5 files changed, 38 insertions(+), 63 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 5408179f325d..4761b6c39626 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -14,9 +14,9 @@ The following query parameters are available: - `"alphabetical"` - Rooms are ordered alphabetically by room name. This is the default. - `"size"` - Rooms are ordered by the number of members. Largest to smallest. * `dir` - Direction of room order. Either `"f"` for forwards or `b` for backwards. Setting -this value to `b` will reverse the above sort order. Defaults to `f`. + this value to `b` will reverse the above sort order. Defaults to `f`. * `search_term` - Filter rooms by their room name. Search term can be contained in any -part of the room name. Defaults to no filtering. + part of the room name. Defaults to no filtering. The following fields are possible in the JSON response body: @@ -27,9 +27,10 @@ The following fields are possible in the JSON response body: - `canonical_alias` - The canonical (main) alias address of the room. - `joined_members` - How many users are currently in the room. * `next_token` - If this field is present, we know that there are potentially -more rooms on the server that did not all fit into this response. We can use -`next_token` to get the "next page" of results. To do so, simply repeat your -request, setting the `from` parameter to the value of `next_token`. + more rooms on the server that did not all fit into this response. + We can use `next_token` to get the "next page" of results. To do + so, simply repeat your request, setting the `from` parameter to + the value of `next_token`. ## Usage @@ -117,9 +118,10 @@ Response: } ``` -The presence of the `next_token` parameter tells us that there are more rooms that stated in -this request, and we need to make another request to get them. To get the next batch of room -results, we repeat our request, setting the `from` parameter to the value of `next_token`. +The presence of the `next_token` parameter tells us that there are more rooms +than returned in this request, and we need to make another request to get them. +To get the next batch of room results, we repeat our request, setting the `from` +parameter to the value of `next_token`. ``` GET /_synapse/admin/rooms?order_by=size&from=100 @@ -149,4 +151,5 @@ Response: } ``` -Once the `next_token` parameter is not present, we know we've reached the end of the list. +Once the `next_token` parameter is not present, we know we've reached the end of +the list. diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index 15e9696f2615..b55f83c3cc3d 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -111,7 +111,10 @@ async def get_rooms_paginate( reverse_order: whether to reverse the room list search_term: a string to filter room names by Returns: - defer.Deferred: a json list[dict[str, Any]] + defer.Deferred: a tuple of list[dict[str, Any]], int|None. A list of + room dicts and an integer, if not None, signifies more results can + be returned if the same call if repeated, substituting the value of + `start` for that of the returned int. """ return await self.store.get_rooms_paginate( start, limit, order_by, reverse_order, search_term diff --git a/synapse/handlers/directory.py b/synapse/handlers/directory.py index 5a082fdc6a40..a07d2f1a17e8 100644 --- a/synapse/handlers/directory.py +++ b/synapse/handlers/directory.py @@ -263,7 +263,7 @@ def get_association(self, room_alias): if not room_id: raise SynapseError( 404, - "Room alias %s not found!" % (room_alias.to_string(),), + "Room alias %s not found" % (room_alias.to_string(),), Codes.NOT_FOUND, ) diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index 6bc6699ecb57..5a4a127fec59 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -182,15 +182,14 @@ async def on_GET(self, request): start = parse_integer(request, "from", default=0) limit = parse_integer(request, "limit", default=100) order_by = parse_string(request, "order_by", default="alphabetical") - search_term = parse_string(request, "search_term") - - direction = parse_string(request, "dir", default="f") - if direction != "f" and direction != "b": + if order_by != "alphabetical" and order_by != "size": raise SynapseError( - 400, "Unknown direction: %s" % direction, errcode=Codes.INVALID_PARAM + 400, + "Unknown value for order_by: %s" % (order_by,), + errcode=Codes.INVALID_PARAM, ) - reverse_order = True if direction == "b" else False + search_term = parse_string(request, "search_term") if search_term == "": raise SynapseError( 400, @@ -198,6 +197,14 @@ async def on_GET(self, request): errcode=Codes.INVALID_PARAM, ) + direction = parse_string(request, "dir", default="f") + if direction != "f" and direction != "b": + raise SynapseError( + 400, "Unknown direction: %s" % (direction,), errcode=Codes.INVALID_PARAM + ) + + reverse_order = True if direction == "b" else False + # Return list of rooms according to parameters rooms, next_token = await self.admin_handler.get_rooms_paginate( start, limit, order_by, reverse_order, search_term diff --git a/synapse/storage/data_stores/main/room.py b/synapse/storage/data_stores/main/room.py index 593414fb996f..b7c484d8dba2 100644 --- a/synapse/storage/data_stores/main/room.py +++ b/synapse/storage/data_stores/main/room.py @@ -300,7 +300,10 @@ async def get_rooms_paginate( reverse_order: whether to reverse the room list search_term: a string to filter room names by Returns: - defer.Deferred: a json list[dict[str, Any]] + defer.Deferred: a tuple of list[dict[str, Any]], int|None. A list of + room dicts and an integer, if not None, signifies more results can + be returned if the same call if repeated, substituting the value of + `start` for that of the returned int. """ # Filter room names by a string where_statement = "" @@ -311,10 +314,12 @@ async def get_rooms_paginate( if order_by == "size": order_by_column = "curr.joined_members" order_by_asc = False - else: + elif order_by == "alphabetical": # Sort alphabetically order_by_column = "state.name" order_by_asc = True + else: + raise StoreError(500, "Incorrect value for order_by provided: %s", order_by) # Whether to return the list in reverse order if reverse_order: @@ -378,7 +383,7 @@ def _get_rooms_paginate_txn(txn): ) rooms = [] - # Refactor rooms into structured dictionary + # Refactor rooms into a structured dictionary for room in room_tuples: rooms.append( { @@ -1144,49 +1149,6 @@ def get_all_new_public_rooms(txn): "get_all_new_public_rooms", get_all_new_public_rooms ) - @defer.inlineCallbacks - def get_all_rooms_with_alias( - self, offset: int, limit: int - ) -> List[Tuple[str, str, str]]: - """Retrieves a list of all rooms the server is in as well as all room - aliases for that room. Rooms are retrieved in alphabetical order by room ID. - - Args: - offset: What to offset the list of rooms by - limit: The maximum number of rooms to return - - Returns: - A list of tuples containing the (room_id, room_name, main room_alias) - """ - - def get_all_rooms_with_alias_txn(txn): - # Retrieve all known room_ids and aliases - sql = """ - SELECT rooms.room_id, room_aliases.room_alias - FROM rooms - INNER JOIN room_aliases on room_aliases.room_id = rooms.room_id - ORDER BY rooms.room_id ASC - LIMIT ? - OFFSET ? - """ - - txn.execute(sql, (limit, offset)) - room_ids_and_aliases = txn.fetchall() - - # Extract all known aliases for a room - room_id_to_alias_list = {} - for room_id, room_alias in room_ids_and_aliases: - if room_id in room_id_to_alias_list: - room_id_to_alias_list[room_id].append(room_alias) - else: - room_id_to_alias_list[room_id] = [room_alias] - - return room_id_to_alias_list - - return self.db.runInteraction( - "get_all_rooms_with_alias", get_all_rooms_with_alias_txn - ) - @defer.inlineCallbacks def block_room(self, room_id, user_id): """Marks the room as blocked. Can be called multiple times. From 9d30f4ae4a254214089bc14ab993b60122246963 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Jan 2020 14:45:02 +0000 Subject: [PATCH 07/22] Fix LIMIT differences in Postgres vs. SQLite --- synapse/storage/data_stores/main/room.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/synapse/storage/data_stores/main/room.py b/synapse/storage/data_stores/main/room.py index b7c484d8dba2..90e66e858e22 100644 --- a/synapse/storage/data_stores/main/room.py +++ b/synapse/storage/data_stores/main/room.py @@ -31,6 +31,7 @@ from synapse.storage._base import SQLBaseStore from synapse.storage.data_stores.main.search import SearchStore from synapse.storage.database import Database +from synapse.storage.engines import PostgresEngine from synapse.types import ThirdPartyInstanceID from synapse.util.caches.descriptors import cached, cachedInlineCallbacks @@ -343,16 +344,26 @@ async def get_rooms_paginate( "ASC" if order_by_asc else "DESC", ) + # We don't want a LIMIT on the count query. SQLite can't handle an OFFSET + # without a LIMIT, however you can hand it `LIMIT -1` to mean "no limit". + # Postgres doesn't like this however, and wants you to eliminate the LIMIT + # keyword altogether + if isinstance(self.database_engine, PostgresEngine): + limit_statement = "" + else: + limit_statement = "LIMIT -1" + # Use a nested SELECT statement as SQL can't count(*) with an OFFSET count_sql = """ SELECT count(*) FROM ( SELECT room_id FROM room_stats_state state %s - LIMIT ? + %s OFFSET ? ) AS get_room_ids """ % ( where_statement, + limit_statement, ) def _get_rooms_paginate_txn(txn): @@ -365,7 +376,7 @@ def _get_rooms_paginate_txn(txn): rows = [row for row in txn] # Execute the count query - sql_values = (-1, start) # Set LIMIT to -1. OFFSET can't be used on its own + sql_values = (start,) if search_term: # Add the search term into the WHERE clause sql_values = (search_term,) + sql_values From ad5edaa6331a1fe5085478a0c752f52deb6054cc Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Jan 2020 15:12:58 +0000 Subject: [PATCH 08/22] debug --- synapse/storage/data_stores/main/room.py | 2 ++ tests/rest/admin/test_admin.py | 1 + 2 files changed, 3 insertions(+) diff --git a/synapse/storage/data_stores/main/room.py b/synapse/storage/data_stores/main/room.py index 90e66e858e22..12071935af20 100644 --- a/synapse/storage/data_stores/main/room.py +++ b/synapse/storage/data_stores/main/room.py @@ -366,6 +366,8 @@ async def get_rooms_paginate( limit_statement, ) + logger.info("info sql: %s count_sql: %s", info_sql, count_sql) + def _get_rooms_paginate_txn(txn): # Execute the data query sql_values = (limit, start) diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index f9e7c68bdd76..b31c8fa7e8f8 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -955,6 +955,7 @@ def _order_test( _order_test("size", [room_id_3, room_id_2, room_id_1]) _order_test("size", [room_id_1, room_id_2, room_id_3], reverse=True) + @unittest.DEBUG def test_search_term(self): """Test that searching for a room works correctly""" # Create two test rooms From 973651d2d485ed1036f521f8238e9a5cd2da9ff5 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Jan 2020 15:38:50 +0000 Subject: [PATCH 09/22] more debugging --- synapse/storage/data_stores/main/room.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/synapse/storage/data_stores/main/room.py b/synapse/storage/data_stores/main/room.py index 12071935af20..39f0e5df4cc7 100644 --- a/synapse/storage/data_stores/main/room.py +++ b/synapse/storage/data_stores/main/room.py @@ -374,6 +374,7 @@ def _get_rooms_paginate_txn(txn): if search_term: # Add the search term into the WHERE clause sql_values = (search_term,) + sql_values + logging.info("info sql_values: %s", sql_values) txn.execute(info_sql, sql_values) rows = [row for row in txn] @@ -382,6 +383,7 @@ def _get_rooms_paginate_txn(txn): if search_term: # Add the search term into the WHERE clause sql_values = (search_term,) + sql_values + logging.info("count sql_values: %s", sql_values) txn.execute(count_sql, sql_values) room_count = txn.fetchone() if room_count: From ed5457c28b0a0e17398c403f0ecef26f8678f3e0 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Fri, 17 Jan 2020 16:27:22 +0000 Subject: [PATCH 10/22] Fix postgres-specific %-related bug --- synapse/storage/data_stores/main/room.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/synapse/storage/data_stores/main/room.py b/synapse/storage/data_stores/main/room.py index 39f0e5df4cc7..57b7f93cc2b5 100644 --- a/synapse/storage/data_stores/main/room.py +++ b/synapse/storage/data_stores/main/room.py @@ -309,7 +309,14 @@ async def get_rooms_paginate( # Filter room names by a string where_statement = "" if search_term: - where_statement = "WHERE state.name LIKE '%' || ? || '%'" + where_statement = "WHERE state.name LIKE ?" + + # Our postgres db driver converts ? -> %s in SQL strings as that's the + # placeholder for postgres. + # HOWEVER, if you put a % into your SQL then everything goes wibbly. + # To get around this, we're going to surround search_term with %'s + # before giving it to the database in python instead + search_term = "%" + search_term + "%" # Set ordering if order_by == "size": From 9ca3bd0d13c3ebd2a50d5b52e4e434d87594201f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2020 13:59:40 +0000 Subject: [PATCH 11/22] Address review comments --- synapse/handlers/admin.py | 11 +++++------ synapse/rest/admin/rooms.py | 5 ++--- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index b55f83c3cc3d..8d73177795e1 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -14,7 +14,7 @@ # limitations under the License. import logging -from typing import List, Optional +from typing import List, Optional, Tuple, Dict, Any from synapse.api.constants import Membership from synapse.events import FrozenEvent @@ -99,7 +99,7 @@ async def get_rooms_paginate( order_by: str, reverse_order: bool, search_term: Optional[str], - ): + ) -> Tuple[List[Dict[str, Any]], Optional[int]]: """Function to retrieve a paginated list of rooms as json. Args: @@ -111,10 +111,9 @@ async def get_rooms_paginate( reverse_order: whether to reverse the room list search_term: a string to filter room names by Returns: - defer.Deferred: a tuple of list[dict[str, Any]], int|None. A list of - room dicts and an integer, if not None, signifies more results can - be returned if the same call if repeated, substituting the value of - `start` for that of the returned int. + A list of room dicts and an integer, if not None, signifies more + results can be returned if the same call if repeated, substituting + the value of `start` for that of the returned int. """ return await self.store.get_rooms_paginate( start, limit, order_by, reverse_order, search_term diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index 5a4a127fec59..ee45efc5c74f 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -13,6 +13,7 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging +import re from synapse.api.constants import Membership from synapse.api.errors import Codes, SynapseError @@ -166,11 +167,9 @@ class ListRoomRestServlet(RestServlet): in a dictionary containing room information. Supports pagination. """ - PATTERNS = historical_admin_path_patterns("/rooms") + PATTERNS = [re.compile("^/_synapse/admin/v1/rooms")] def __init__(self, hs): - self.hs = hs - self.store = hs.get_datastore() self.auth = hs.get_auth() self.admin_handler = hs.get_handlers().admin_handler From 5205820f99bf2200ceefdc77bb4b81f301413fba Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2020 17:59:23 +0000 Subject: [PATCH 12/22] Address review comments. Return total rooms --- docs/admin_api/rooms.md | 19 +++-- synapse/handlers/admin.py | 29 +------ synapse/rest/admin/_base.py | 14 ++++ synapse/rest/admin/rooms.py | 29 +++++-- synapse/storage/data_stores/main/room.py | 101 +++++++++-------------- tests/rest/admin/test_admin.py | 21 +---- 6 files changed, 92 insertions(+), 121 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 4761b6c39626..84bf870f7bb1 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -11,9 +11,9 @@ The following query parameters are available: * `from` - Offset in the returned list. Defaults to `0`. * `limit` - Maximum amount of rooms to return. Defaults to `100`. * `order_by` - The method in which to sort the returned list of rooms. Valid values are: - - `"alphabetical"` - Rooms are ordered alphabetically by room name. This is the default. - - `"size"` - Rooms are ordered by the number of members. Largest to smallest. -* `dir` - Direction of room order. Either `"f"` for forwards or `b` for backwards. Setting + - `alphabetical` - Rooms are ordered alphabetically by room name. This is the default. + - `size` - Rooms are ordered by the number of members. Largest to smallest. +* `dir` - Direction of room order. Either `f` for forwards or `b` for backwards. Setting this value to `b` will reverse the above sort order. Defaults to `f`. * `search_term` - Filter rooms by their room name. Search term can be contained in any part of the room name. Defaults to no filtering. @@ -26,6 +26,7 @@ The following fields are possible in the JSON response body: - `name` - The name of the room. - `canonical_alias` - The canonical (main) alias address of the room. - `joined_members` - How many users are currently in the room. +* `total_rooms` - The total number of rooms this query can return. * `next_token` - If this field is present, we know that there are potentially more rooms on the server that did not all fit into this response. We can use `next_token` to get the "next page" of results. To do @@ -60,7 +61,8 @@ Response: "canonical_alias": "#twim:matrix.org", "joined_members": 314 } - ] + ], + "total_rooms": 10 } ``` @@ -83,7 +85,8 @@ Response: "canonical_alias": "#twim:matrix.org", "joined_members": 314 } - ] + ], + "total_rooms": 1 } ``` @@ -114,6 +117,7 @@ Response: "joined_members": 314 } ], + "total_rooms": 150 "next_token": 100 } ``` @@ -140,14 +144,15 @@ Response: "canonical_alias": "#musictheory:matrix.org", "joined_members": 127 }, - ... (65 hidden items) ... + ... (48 hidden items) ... { "room_id": "!twcBhHVdZlQWuuxBhN:termina.org.uk", "name": "weechat-matrix", "canonical_alias": "#weechat-matrix:termina.org.uk", "joined_members": 137 } - ] + ], + "total_rooms": 150 } ``` diff --git a/synapse/handlers/admin.py b/synapse/handlers/admin.py index 8d73177795e1..60a7c938bc84 100644 --- a/synapse/handlers/admin.py +++ b/synapse/handlers/admin.py @@ -14,7 +14,7 @@ # limitations under the License. import logging -from typing import List, Optional, Tuple, Dict, Any +from typing import List from synapse.api.constants import Membership from synapse.events import FrozenEvent @@ -92,33 +92,6 @@ async def get_users_paginate(self, start, limit, name, guests, deactivated): return ret - async def get_rooms_paginate( - self, - start: int, - limit: int, - order_by: str, - reverse_order: bool, - search_term: Optional[str], - ) -> Tuple[List[Dict[str, Any]], Optional[int]]: - """Function to retrieve a paginated list of rooms as json. - - Args: - start: offset in the list - limit: maximum amount of rooms to retrieve - order_by: the sort order of the returned list. valid values: - * alphabetical (default) - * size - reverse_order: whether to reverse the room list - search_term: a string to filter room names by - Returns: - A list of room dicts and an integer, if not None, signifies more - results can be returned if the same call if repeated, substituting - the value of `start` for that of the returned int. - """ - return await self.store.get_rooms_paginate( - start, limit, order_by, reverse_order, search_term - ) - async def search_users(self, term): """Function to search users list for one or more users with the matched term. diff --git a/synapse/rest/admin/_base.py b/synapse/rest/admin/_base.py index afd064720564..d71eb2f62fed 100644 --- a/synapse/rest/admin/_base.py +++ b/synapse/rest/admin/_base.py @@ -14,6 +14,8 @@ # limitations under the License. import re +from sre_parse import Pattern +from typing import List from synapse.api.errors import AuthError @@ -40,6 +42,18 @@ def historical_admin_path_patterns(path_regex): ) +def admin_patterns(path_regex: str) -> List[Pattern]: + """Returns the list of patterns for an admin endpoint + + Args: + path_regex: The regex string to match. This should NOT have a ^ + as this will be prefixed. + """ + admin_prefix = "^/_synapse/admin/v1" + regex = re.compile(admin_prefix + path_regex) + return [regex] + + async def assert_requester_is_admin(auth, request): """Verify that the requester is an admin user diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index ee45efc5c74f..ab86c2ec02fb 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -13,7 +13,6 @@ # See the License for the specific language governing permissions and # limitations under the License. import logging -import re from synapse.api.constants import Membership from synapse.api.errors import Codes, SynapseError @@ -25,9 +24,11 @@ parse_string, ) from synapse.rest.admin._base import ( + admin_patterns, assert_user_is_admin, historical_admin_path_patterns, ) +from synapse.storage.data_stores.main.room import RoomSortOrder from synapse.types import create_requester from synapse.util.async_helpers import maybe_awaitable @@ -167,9 +168,10 @@ class ListRoomRestServlet(RestServlet): in a dictionary containing room information. Supports pagination. """ - PATTERNS = [re.compile("^/_synapse/admin/v1/rooms")] + PATTERNS = admin_patterns("/rooms") def __init__(self, hs): + self.store = hs.get_datastore() self.auth = hs.get_auth() self.admin_handler = hs.get_handlers().admin_handler @@ -181,7 +183,10 @@ async def on_GET(self, request): start = parse_integer(request, "from", default=0) limit = parse_integer(request, "limit", default=100) order_by = parse_string(request, "order_by", default="alphabetical") - if order_by != "alphabetical" and order_by != "size": + if RoomSortOrder(order_by) not in ( + RoomSortOrder.ALPHABETICAL, + RoomSortOrder.SIZE, + ): raise SynapseError( 400, "Unknown value for order_by: %s" % (order_by,), @@ -197,7 +202,7 @@ async def on_GET(self, request): ) direction = parse_string(request, "dir", default="f") - if direction != "f" and direction != "b": + if direction not in ("f", "b"): raise SynapseError( 400, "Unknown direction: %s" % (direction,), errcode=Codes.INVALID_PARAM ) @@ -205,12 +210,18 @@ async def on_GET(self, request): reverse_order = True if direction == "b" else False # Return list of rooms according to parameters - rooms, next_token = await self.admin_handler.get_rooms_paginate( + rooms, total_rooms = await self.store.get_rooms_paginate( start, limit, order_by, reverse_order, search_term ) - response = {"rooms": rooms} - - if next_token: - response["next_token"] = next_token + response = { + "rooms": rooms, + "total_rooms": total_rooms, + } + + # Are there more rooms to paginate through after this? + if (start + limit) < total_rooms: + # There are. Calculate where the query should start from next time + # to get the next part of the list + response["next_token"] = start + limit return 200, response diff --git a/synapse/storage/data_stores/main/room.py b/synapse/storage/data_stores/main/room.py index 57b7f93cc2b5..1b23178be7bd 100644 --- a/synapse/storage/data_stores/main/room.py +++ b/synapse/storage/data_stores/main/room.py @@ -18,7 +18,8 @@ import logging import re from abc import abstractmethod -from typing import List, Optional, Tuple +from enum import Enum +from typing import Any, Dict, List, Optional, Tuple from six import integer_types @@ -31,7 +32,6 @@ from synapse.storage._base import SQLBaseStore from synapse.storage.data_stores.main.search import SearchStore from synapse.storage.database import Database -from synapse.storage.engines import PostgresEngine from synapse.types import ThirdPartyInstanceID from synapse.util.caches.descriptors import cached, cachedInlineCallbacks @@ -47,6 +47,18 @@ ) +class RoomSortOrder(Enum): + """ + Enum to define the sorting method used when returning rooms with get_rooms_paginate + + ALPHABETICAL = sort rooms alphabetically by name + SIZE = sort rooms by membership size, highest to lowest + """ + + ALPHABETICAL = "alphabetical" + SIZE = "size" + + class RoomWorkerStore(SQLBaseStore): def __init__(self, database: Database, db_conn, hs): super(RoomWorkerStore, self).__init__(database, db_conn, hs) @@ -286,18 +298,16 @@ async def get_rooms_paginate( self, start: int, limit: int, - order_by: str, + order_by: RoomSortOrder, reverse_order: bool, search_term: Optional[str], - ): + ) -> Tuple[List[Dict[str, Any]], int]: """Function to retrieve a paginated list of rooms as json. Args: start: offset in the list limit: maximum amount of rooms to retrieve - order_by: the sort order of the returned list. valid values: - * alphabetical (default) - * size + order_by: the sort order of the returned list reverse_order: whether to reverse the room list search_term: a string to filter room names by Returns: @@ -319,15 +329,17 @@ async def get_rooms_paginate( search_term = "%" + search_term + "%" # Set ordering - if order_by == "size": + if RoomSortOrder(order_by) == RoomSortOrder.SIZE: order_by_column = "curr.joined_members" order_by_asc = False - elif order_by == "alphabetical": + elif RoomSortOrder(order_by) == RoomSortOrder.ALPHABETICAL: # Sort alphabetically order_by_column = "state.name" order_by_asc = True else: - raise StoreError(500, "Incorrect value for order_by provided: %s", order_by) + raise StoreError( + 500, "Incorrect value for order_by provided: %s" % order_by + ) # Whether to return the list in reverse order if reverse_order: @@ -340,7 +352,7 @@ async def get_rooms_paginate( info_sql = """ SELECT state.room_id, state.name, state.canonical_alias, curr.joined_members FROM room_stats_state state - LEFT JOIN room_stats_current curr ON state.room_id = curr.room_id + INNER JOIN room_stats_current curr USING (room_id) %s ORDER BY %s %s LIMIT ? @@ -351,79 +363,48 @@ async def get_rooms_paginate( "ASC" if order_by_asc else "DESC", ) - # We don't want a LIMIT on the count query. SQLite can't handle an OFFSET - # without a LIMIT, however you can hand it `LIMIT -1` to mean "no limit". - # Postgres doesn't like this however, and wants you to eliminate the LIMIT - # keyword altogether - if isinstance(self.database_engine, PostgresEngine): - limit_statement = "" - else: - limit_statement = "LIMIT -1" - # Use a nested SELECT statement as SQL can't count(*) with an OFFSET count_sql = """ SELECT count(*) FROM ( SELECT room_id FROM room_stats_state state %s - %s - OFFSET ? ) AS get_room_ids """ % ( where_statement, - limit_statement, ) - logger.info("info sql: %s count_sql: %s", info_sql, count_sql) - def _get_rooms_paginate_txn(txn): # Execute the data query sql_values = (limit, start) if search_term: # Add the search term into the WHERE clause sql_values = (search_term,) + sql_values - logging.info("info sql_values: %s", sql_values) txn.execute(info_sql, sql_values) - rows = [row for row in txn] + + # Refactor room query data into a structured dictionary + rooms = [] + for room in txn: + rooms.append( + { + "room_id": room[0], + "name": room[1], + "canonical_alias": room[2], + "joined_members": room[3], + } + ) # Execute the count query - sql_values = (start,) - if search_term: - # Add the search term into the WHERE clause - sql_values = (search_term,) + sql_values - logging.info("count sql_values: %s", sql_values) + + # Add the search term into the WHERE clause if present + sql_values = (search_term,) if search_term else () txn.execute(count_sql, sql_values) - room_count = txn.fetchone() - if room_count: - return rows, room_count[0] - # room_count can be None if there are no rows and OFFSET > 1 - # Set to 0 instead - return rows, 0 + room_count = txn.fetchone() + return rooms, room_count[0] - room_tuples, total_rooms = await self.db.runInteraction( + return await self.db.runInteraction( "get_rooms_paginate", _get_rooms_paginate_txn, ) - rooms = [] - - # Refactor rooms into a structured dictionary - for room in room_tuples: - rooms.append( - { - "room_id": room[0], - "name": room[1], - "canonical_alias": room[2], - "joined_members": room[3], - } - ) - - # Are there more rooms to paginate through after this? - next_token = None - if total_rooms > len(room_tuples): - # There are. Calculate where the query should start from next time to get - # the next part of the list - next_token = start + limit - - return rooms, next_token @cachedInlineCallbacks(max_entries=10000) def get_ratelimit_for_user(self, user_id): diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index b31c8fa7e8f8..5b3abae94e77 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -736,13 +736,7 @@ def test_list_rooms(self): # Check their room_ids match returned_room_ids = [room["room_id"] for room in channel.json_body["rooms"]] - common_room_ids = set(room_ids) & set(returned_room_ids) - self.assertEqual( - len(common_room_ids), - total_rooms, - msg="Different room_ids than expected returned. " - "Expected:\n%s\nReturned:\n%s" % (room_ids, returned_room_ids), - ) + self.assertEqual(room_ids, returned_room_ids) # Check that all fields are available for r in channel.json_body["rooms"]: @@ -751,7 +745,7 @@ def test_list_rooms(self): self.assertIn("joined_members", r) # We shouldn't receive a next token here as there's no further rooms to show - self.assertTrue("next_token" not in channel.json_body) + self.assertNotIn("next_token", channel.json_body) def test_list_rooms_pagination(self): """Test that we can get a full list of rooms through pagination""" @@ -799,17 +793,11 @@ def test_list_rooms_pagination(self): self.assertEqual( run_count, 3, - msg="Should've queried 3 times for 5 rooms with " "limit 2 per query", + msg="Should've queried 3 times for 5 rooms with limit 2 per query", ) # Check that we received all of the room ids - common_room_ids = set(room_ids) & set(returned_room_ids) - self.assertEqual( - len(common_room_ids), - total_rooms, - msg="Different room_ids than expected returned. " - "Expected:\n%s\nReturned:\n%s" % (room_ids, returned_room_ids), - ) + self.assertEqual(room_ids, returned_room_ids) url = "/_synapse/admin/v1/rooms?from=%d&limit=%d" % (start, limit) request, channel = self.make_request( @@ -955,7 +943,6 @@ def _order_test( _order_test("size", [room_id_3, room_id_2, room_id_1]) _order_test("size", [room_id_1, room_id_2, room_id_3], reverse=True) - @unittest.DEBUG def test_search_term(self): """Test that searching for a room works correctly""" # Create two test rooms From b449efdd7af0dfcbc19649821dcc20be21caee76 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2020 18:00:54 +0000 Subject: [PATCH 13/22] Small docs clarification --- docs/admin_api/rooms.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 84bf870f7bb1..3f3c4edb5555 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -54,7 +54,7 @@ Response: "canonical_alias": "#matrix:matrix.org", "joined_members": 8326 }, - ... + ... (8 hidden items) ... { "room_id": "!xYvNcQPhnkrdUmYczI:matrix.org", "name": "This Week In Matrix (TWIM)", From 980cbd38acfdd6f343228a85b2064029bcd4bcff Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2020 18:01:55 +0000 Subject: [PATCH 14/22] Indentation --- synapse/rest/admin/_base.py | 2 +- synapse/rest/client/v2_alpha/_base.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/admin/_base.py b/synapse/rest/admin/_base.py index d71eb2f62fed..90a1553a66aa 100644 --- a/synapse/rest/admin/_base.py +++ b/synapse/rest/admin/_base.py @@ -47,7 +47,7 @@ def admin_patterns(path_regex: str) -> List[Pattern]: Args: path_regex: The regex string to match. This should NOT have a ^ - as this will be prefixed. + as this will be prefixed. """ admin_prefix = "^/_synapse/admin/v1" regex = re.compile(admin_prefix + path_regex) diff --git a/synapse/rest/client/v2_alpha/_base.py b/synapse/rest/client/v2_alpha/_base.py index 2a3f4dd58f28..bc11b4dda4ab 100644 --- a/synapse/rest/client/v2_alpha/_base.py +++ b/synapse/rest/client/v2_alpha/_base.py @@ -32,7 +32,7 @@ def client_patterns(path_regex, releases=(0,), unstable=True, v1=False): Args: path_regex (str): The regex string to match. This should NOT have a ^ - as this will be prefixed. + as this will be prefixed. Returns: SRE_Pattern """ From 1cfd5e18b543c2b68d2abae68253f8fca937a422 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2020 18:03:34 +0000 Subject: [PATCH 15/22] admin_api creation method refactor --- synapse/rest/admin/_base.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/rest/admin/_base.py b/synapse/rest/admin/_base.py index 90a1553a66aa..96287801412e 100644 --- a/synapse/rest/admin/_base.py +++ b/synapse/rest/admin/_base.py @@ -50,8 +50,8 @@ def admin_patterns(path_regex: str) -> List[Pattern]: as this will be prefixed. """ admin_prefix = "^/_synapse/admin/v1" - regex = re.compile(admin_prefix + path_regex) - return [regex] + patterns = [re.compile(admin_prefix + path_regex)] + return patterns async def assert_requester_is_admin(auth, request): From f91e692b54ac1cf052ee345bf6b7367299c564a5 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2020 18:05:58 +0000 Subject: [PATCH 16/22] Fix docstring --- synapse/storage/data_stores/main/room.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/synapse/storage/data_stores/main/room.py b/synapse/storage/data_stores/main/room.py index 1b23178be7bd..d968803ad2fb 100644 --- a/synapse/storage/data_stores/main/room.py +++ b/synapse/storage/data_stores/main/room.py @@ -311,10 +311,8 @@ async def get_rooms_paginate( reverse_order: whether to reverse the room list search_term: a string to filter room names by Returns: - defer.Deferred: a tuple of list[dict[str, Any]], int|None. A list of - room dicts and an integer, if not None, signifies more results can - be returned if the same call if repeated, substituting the value of - `start` for that of the returned int. + A list of room dicts and an integer representing the total number of + rooms that exist given this query """ # Filter room names by a string where_statement = "" From 93b2212fa5397e6678c5b9e948ef907f422c4fb9 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2020 19:08:25 +0000 Subject: [PATCH 17/22] Add 'total_rooms' key to tests --- tests/rest/admin/test_admin.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index 5b3abae94e77..186819d6c636 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -744,6 +744,9 @@ def test_list_rooms(self): self.assertIn("canonical_alias", r) self.assertIn("joined_members", r) + # Check that the correct number of total rooms was returned + self.assertEqual(channel.json_body["total_rooms"], total_rooms) + # We shouldn't receive a next token here as there's no further rooms to show self.assertNotIn("next_token", channel.json_body) @@ -782,6 +785,9 @@ def test_list_rooms_pagination(self): for r in channel.json_body["rooms"]: returned_room_ids.append(r["room_id"]) + # Check that the correct number of total rooms was returned + self.assertEqual(channel.json_body["total_rooms"], total_rooms) + if "next_token" not in channel.json_body: # We have reached the end of the list should_repeat = False @@ -865,6 +871,9 @@ def test_correct_room_attributes(self): # Check that only one room was returned self.assertEqual(len(rooms), 1) + # And that the value of the total_rooms key was correct + self.assertEqual(channel.json_body["total_rooms"], 1) + # Check that there is no `next_token` self.assertNotIn("next_token", channel.json_body) @@ -932,6 +941,9 @@ def _order_test( self.assertTrue("rooms" in channel.json_body) rooms = channel.json_body["rooms"] + # Check for the correct total_rooms value + self.assertEqual(channel.json_body["total_rooms"], 3) + # Check that rooms were returned in alphabetical order returned_order = [r["room_id"] for r in rooms] self.assertListEqual(expected_room_list, returned_order) # order is checked @@ -988,7 +1000,9 @@ def _search_test( rooms = channel.json_body["rooms"] # Check that the expected number of rooms were returned - self.assertEqual(len(rooms), 1 if expected_room_id else 0) + expected_room_count = 1 if expected_room_id else 0 + self.assertEqual(len(rooms), expected_room_count) + self.assertEqual(channel.json_body["total_rooms"], expected_room_count) if expected_room_id: # Check that the first returned room id is correct From 1c5ff45c7f38a7e38acd3edeb801f6e29782a92f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2020 19:20:12 +0000 Subject: [PATCH 18/22] Ensure pagination test returns rooms in a predicatable order --- tests/rest/admin/test_admin.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index 186819d6c636..c4ddc62e7e6f 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -761,9 +761,14 @@ def test_list_rooms_pagination(self): ) room_ids.append(room_id) - returned_room_ids = [] + # Set the name of the rooms so we get a consistent returned ordering + for idx, room_id in enumerate(room_ids): + self.helper.send_state( + room_id, "m.room.name", {"name": str(idx)}, tok=self.admin_user_tok, + ) # Request the list of rooms + returned_room_ids = [] start = 0 limit = 2 @@ -772,7 +777,11 @@ def test_list_rooms_pagination(self): while should_repeat: run_count += 1 - url = "/_synapse/admin/v1/rooms?from=%d&limit=%d" % (start, limit) + url = "/_synapse/admin/v1/rooms?from=%d&limit=%d&order_by=%s" % ( + start, + limit, + "alphabetical", + ) request, channel = self.make_request( "GET", url.encode("ascii"), access_token=self.admin_user_tok, ) From 6bebc12228a4a7b52c03b728feb15043aba815ab Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Mon, 20 Jan 2020 19:52:13 +0000 Subject: [PATCH 19/22] Can't use regex pattern in method type definition --- synapse/rest/admin/_base.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/synapse/rest/admin/_base.py b/synapse/rest/admin/_base.py index 96287801412e..459482eb6d53 100644 --- a/synapse/rest/admin/_base.py +++ b/synapse/rest/admin/_base.py @@ -14,8 +14,6 @@ # limitations under the License. import re -from sre_parse import Pattern -from typing import List from synapse.api.errors import AuthError @@ -42,12 +40,15 @@ def historical_admin_path_patterns(path_regex): ) -def admin_patterns(path_regex: str) -> List[Pattern]: +def admin_patterns(path_regex: str): """Returns the list of patterns for an admin endpoint Args: path_regex: The regex string to match. This should NOT have a ^ as this will be prefixed. + + Returns: + A list of regex patterns. """ admin_prefix = "^/_synapse/admin/v1" patterns = [re.compile(admin_prefix + path_regex)] From 8374cb134440d594e1976cfcf98111143dc1f82f Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 21 Jan 2020 11:02:02 +0000 Subject: [PATCH 20/22] Add offset parameter to the response --- docs/admin_api/rooms.md | 15 ++++++++++++--- synapse/rest/admin/rooms.py | 2 ++ tests/rest/admin/test_admin.py | 20 ++++++++++++++++++++ 3 files changed, 34 insertions(+), 3 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 3f3c4edb5555..1147ee306758 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -26,7 +26,12 @@ The following fields are possible in the JSON response body: - `name` - The name of the room. - `canonical_alias` - The canonical (main) alias address of the room. - `joined_members` - How many users are currently in the room. -* `total_rooms` - The total number of rooms this query can return. +* `offset` - The current pagination offset in rooms. This parameter should be + used instead of `next_token` for room offset as `next_token` is + not intended to be parsed. +* `total_rooms` - The total number of rooms this query can return. Using this + and `offset`, you have enough information to know the current + progression through the list. * `next_token` - If this field is present, we know that there are potentially more rooms on the server that did not all fit into this response. We can use `next_token` to get the "next page" of results. To do @@ -62,6 +67,7 @@ Response: "joined_members": 314 } ], + "offset": 0, "total_rooms": 10 } ``` @@ -86,6 +92,7 @@ Response: "joined_members": 314 } ], + "offset": 0, "total_rooms": 1 } ``` @@ -117,6 +124,7 @@ Response: "joined_members": 314 } ], + "offset": 0, "total_rooms": 150 "next_token": 100 } @@ -152,9 +160,10 @@ Response: "joined_members": 137 } ], + "offset": 0, "total_rooms": 150 } ``` -Once the `next_token` parameter is not present, we know we've reached the end of -the list. +Once the `next_token` parameter is no longer present, we know we've reached the +end of the list. diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index ab86c2ec02fb..01b1733fe5ce 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -214,6 +214,8 @@ async def on_GET(self, request): start, limit, order_by, reverse_order, search_term ) response = { + # next_token should be opaque, so return a value the client can parse + "offset": start, "rooms": rooms, "total_rooms": total_rooms, } diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index c4ddc62e7e6f..f2ed78772e3e 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -747,6 +747,10 @@ def test_list_rooms(self): # Check that the correct number of total rooms was returned self.assertEqual(channel.json_body["total_rooms"], total_rooms) + # Check that the offset is correct + # Should be 0 as we aren't paginating + self.assertEqual(channel.json_body["offset"], 0) + # We shouldn't receive a next token here as there's no further rooms to show self.assertNotIn("next_token", channel.json_body) @@ -797,6 +801,10 @@ def test_list_rooms_pagination(self): # Check that the correct number of total rooms was returned self.assertEqual(channel.json_body["total_rooms"], total_rooms) + # Check that the offset is correct + # We're only getting 2 rooms each page, so should be 2 * last run_count + self.assertEqual(channel.json_body["offset"], 2 * (run_count - 1)) + if "next_token" not in channel.json_body: # We have reached the end of the list should_repeat = False @@ -883,6 +891,10 @@ def test_correct_room_attributes(self): # And that the value of the total_rooms key was correct self.assertEqual(channel.json_body["total_rooms"], 1) + # Check that the offset is correct + # We're not paginating, so should be 0 + self.assertEqual(channel.json_body["offset"], 0) + # Check that there is no `next_token` self.assertNotIn("next_token", channel.json_body) @@ -953,6 +965,10 @@ def _order_test( # Check for the correct total_rooms value self.assertEqual(channel.json_body["total_rooms"], 3) + # Check that the offset is correct + # We're not paginating, so should be 0 + self.assertEqual(channel.json_body["offset"], 0) + # Check that rooms were returned in alphabetical order returned_order = [r["room_id"] for r in rooms] self.assertListEqual(expected_room_list, returned_order) # order is checked @@ -1013,6 +1029,10 @@ def _search_test( self.assertEqual(len(rooms), expected_room_count) self.assertEqual(channel.json_body["total_rooms"], expected_room_count) + # Check that the offset is correct + # We're not paginating, so should be 0 + self.assertEqual(channel.json_body["offset"], 0) + if expected_room_id: # Check that the first returned room id is correct r = rooms[0] From 7be87aa539530725e83901df0c236f4e2e930ad2 Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 21 Jan 2020 13:12:22 +0000 Subject: [PATCH 21/22] Add prev_batch. next_token -> next_batch --- docs/admin_api/rooms.md | 12 ++++++++---- synapse/rest/admin/rooms.py | 12 +++++++++++- tests/rest/admin/test_admin.py | 32 +++++++++++++++++++++++++++----- 3 files changed, 46 insertions(+), 10 deletions(-) diff --git a/docs/admin_api/rooms.md b/docs/admin_api/rooms.md index 1147ee306758..082721ea9570 100644 --- a/docs/admin_api/rooms.md +++ b/docs/admin_api/rooms.md @@ -32,11 +32,14 @@ The following fields are possible in the JSON response body: * `total_rooms` - The total number of rooms this query can return. Using this and `offset`, you have enough information to know the current progression through the list. -* `next_token` - If this field is present, we know that there are potentially +* `next_batch` - If this field is present, we know that there are potentially more rooms on the server that did not all fit into this response. - We can use `next_token` to get the "next page" of results. To do + We can use `next_batch` to get the "next page" of results. To do so, simply repeat your request, setting the `from` parameter to - the value of `next_token`. + the value of `next_batch`. +* `prev_batch` - If this field is present, it is possible to paginate backwards. + Use `prev_batch` for the `from` value in the next request to + get the "previous page" of results. ## Usage @@ -160,7 +163,8 @@ Response: "joined_members": 137 } ], - "offset": 0, + "offset": 100, + "prev_batch": 0, "total_rooms": 150 } ``` diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index 01b1733fe5ce..facb0bc1f391 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -224,6 +224,16 @@ async def on_GET(self, request): if (start + limit) < total_rooms: # There are. Calculate where the query should start from next time # to get the next part of the list - response["next_token"] = start + limit + response["next_batch"] = start + limit + + # Is it possible to paginate backwards? Check if we currently have an + # offset + if start > 0: + if start > limit: + # Going back one iteration won't take us to the start. + # Calculate new offset + response["prev_batch"] = start - limit + else: + response["prev_batch"] = 0 return 200, response diff --git a/tests/rest/admin/test_admin.py b/tests/rest/admin/test_admin.py index f2ed78772e3e..c1a28f5a9e9f 100644 --- a/tests/rest/admin/test_admin.py +++ b/tests/rest/admin/test_admin.py @@ -751,8 +751,11 @@ def test_list_rooms(self): # Should be 0 as we aren't paginating self.assertEqual(channel.json_body["offset"], 0) + # Check that the prev_batch parameter is not present + self.assertNotIn("prev_batch", channel.json_body) + # We shouldn't receive a next token here as there's no further rooms to show - self.assertNotIn("next_token", channel.json_body) + self.assertNotIn("next_batch", channel.json_body) def test_list_rooms_pagination(self): """Test that we can get a full list of rooms through pagination""" @@ -805,12 +808,16 @@ def test_list_rooms_pagination(self): # We're only getting 2 rooms each page, so should be 2 * last run_count self.assertEqual(channel.json_body["offset"], 2 * (run_count - 1)) - if "next_token" not in channel.json_body: + if run_count > 1: + # Check the value of prev_batch is correct + self.assertEqual(channel.json_body["prev_batch"], 2 * (run_count - 2)) + + if "next_batch" not in channel.json_body: # We have reached the end of the list should_repeat = False else: # Make another query with an updated start value - start = channel.json_body["next_token"] + start = channel.json_body["next_batch"] # We should've queried the endpoint 3 times self.assertEqual( @@ -895,8 +902,11 @@ def test_correct_room_attributes(self): # We're not paginating, so should be 0 self.assertEqual(channel.json_body["offset"], 0) - # Check that there is no `next_token` - self.assertNotIn("next_token", channel.json_body) + # Check that there is no `prev_batch` + self.assertNotIn("prev_batch", channel.json_body) + + # Check that there is no `next_batch` + self.assertNotIn("next_batch", channel.json_body) # Check that all provided attributes are set r = rooms[0] @@ -969,6 +979,12 @@ def _order_test( # We're not paginating, so should be 0 self.assertEqual(channel.json_body["offset"], 0) + # Check that there is no `prev_batch` + self.assertNotIn("prev_batch", channel.json_body) + + # Check that there is no `next_batch` + self.assertNotIn("next_batch", channel.json_body) + # Check that rooms were returned in alphabetical order returned_order = [r["room_id"] for r in rooms] self.assertListEqual(expected_room_list, returned_order) # order is checked @@ -1033,6 +1049,12 @@ def _search_test( # We're not paginating, so should be 0 self.assertEqual(channel.json_body["offset"], 0) + # Check that there is no `prev_batch` + self.assertNotIn("prev_batch", channel.json_body) + + # Check that there is no `next_batch` + self.assertNotIn("next_batch", channel.json_body) + if expected_room_id: # Check that the first returned room id is correct r = rooms[0] From 8c7f6681f4fd82d23f9ca884d78641761a56e54c Mon Sep 17 00:00:00 2001 From: Andrew Morgan Date: Tue, 21 Jan 2020 13:18:47 +0000 Subject: [PATCH 22/22] Don't raise exception on incorrect order_by value --- synapse/rest/admin/rooms.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/rest/admin/rooms.py b/synapse/rest/admin/rooms.py index facb0bc1f391..f9b8c0a4f0f3 100644 --- a/synapse/rest/admin/rooms.py +++ b/synapse/rest/admin/rooms.py @@ -183,9 +183,9 @@ async def on_GET(self, request): start = parse_integer(request, "from", default=0) limit = parse_integer(request, "limit", default=100) order_by = parse_string(request, "order_by", default="alphabetical") - if RoomSortOrder(order_by) not in ( - RoomSortOrder.ALPHABETICAL, - RoomSortOrder.SIZE, + if order_by not in ( + RoomSortOrder.ALPHABETICAL.value, + RoomSortOrder.SIZE.value, ): raise SynapseError( 400,