From 942a0556fc0d034c5f17cad241f3877bf9ba7936 Mon Sep 17 00:00:00 2001 From: Daniel Sonck Date: Thu, 13 Jan 2022 11:28:14 +0100 Subject: [PATCH 1/5] Make pagination of rooms in admin api stable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Always add state.room_id after the configurable ORDER BY. Otherwise, for any sort, certain pages can contain results from other pages. (Especially when sorting by creator, since there may be many rooms by the same creator) Signed-off-by: Daniël Sonck --- changelog.d/11737.bugfix | 1 + synapse/storage/databases/main/room.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog.d/11737.bugfix diff --git a/changelog.d/11737.bugfix b/changelog.d/11737.bugfix new file mode 100644 index 000000000000..a293d1cfec0c --- /dev/null +++ b/changelog.d/11737.bugfix @@ -0,0 +1 @@ +Make the list rooms admin api sort stable. Contributed by Daniël Sonck. \ No newline at end of file diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index c0e837854a32..3f6fd81b3e65 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -552,7 +552,7 @@ async def get_rooms_paginate( INNER JOIN room_stats_current curr USING (room_id) INNER JOIN rooms USING (room_id) %s - ORDER BY %s %s + ORDER BY %s %s, state.room_id ASC LIMIT ? OFFSET ? """ % ( From 2fdeced12b45dc536ef1e89ce4e496bc6f2f9f74 Mon Sep 17 00:00:00 2001 From: Daniel Sonck Date: Thu, 13 Jan 2022 19:54:32 +0100 Subject: [PATCH 2/5] Fix failing tests Fix test by checking against a sorted room_id array. If there are multiple rooms with equal order_by values, individual rooms that share this same value are ordered by room_id. "version", "creator", "encryption" "federatable", "join_rules", "guest_access", "history_visibility" all have multiple rooms with the same value, thus need this specifically ordered id list. --- tests/rest/admin/test_room.py | 36 ++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index d2c8781cd4b9..1aff3626367c 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -1089,6 +1089,8 @@ def test_list_rooms(self) -> None: ) room_ids.append(room_id) + room_ids.sort() + # Request the list of rooms url = "/_synapse/admin/v1/rooms" channel = self.make_request( @@ -1360,6 +1362,10 @@ def _order_test( 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) + # Also create a list sorted by IDs for properties that are equal (and thus sorted by room_id) + all_ids = [room_id_1, room_id_2, room_id_3] + all_ids.sort() + # Set room names in alphabetical order. room 1 -> A, 2 -> B, 3 -> C self.helper.send_state( room_id_1, @@ -1413,31 +1419,31 @@ def _order_test( "joined_local_members", [room_id_1, room_id_2, room_id_3], reverse=True ) - _order_test("version", [room_id_1, room_id_2, room_id_3]) - _order_test("version", [room_id_1, room_id_2, room_id_3], reverse=True) + _order_test("version", all_ids) + _order_test("version", all_ids, reverse=True) - _order_test("creator", [room_id_1, room_id_2, room_id_3]) - _order_test("creator", [room_id_1, room_id_2, room_id_3], reverse=True) + _order_test("creator", all_ids) + _order_test("creator", all_ids, reverse=True) - _order_test("encryption", [room_id_1, room_id_2, room_id_3]) - _order_test("encryption", [room_id_1, room_id_2, room_id_3], reverse=True) + _order_test("encryption", all_ids) + _order_test("encryption", all_ids, reverse=True) - _order_test("federatable", [room_id_1, room_id_2, room_id_3]) - _order_test("federatable", [room_id_1, room_id_2, room_id_3], reverse=True) + _order_test("federatable", all_ids) + _order_test("federatable", all_ids, reverse=True) - _order_test("public", [room_id_1, room_id_2, room_id_3]) + _order_test("public", all_ids) # Different sort order of SQlite and PostreSQL # _order_test("public", [room_id_3, room_id_2, room_id_1], reverse=True) - _order_test("join_rules", [room_id_1, room_id_2, room_id_3]) - _order_test("join_rules", [room_id_1, room_id_2, room_id_3], reverse=True) + _order_test("join_rules", all_ids) + _order_test("join_rules", all_ids, reverse=True) - _order_test("guest_access", [room_id_1, room_id_2, room_id_3]) - _order_test("guest_access", [room_id_1, room_id_2, room_id_3], reverse=True) + _order_test("guest_access", all_ids) + _order_test("guest_access", all_ids, reverse=True) - _order_test("history_visibility", [room_id_1, room_id_2, room_id_3]) + _order_test("history_visibility", all_ids) _order_test( - "history_visibility", [room_id_1, room_id_2, room_id_3], reverse=True + "history_visibility", all_ids, reverse=True ) _order_test("state_events", [room_id_3, room_id_2, room_id_1]) From d2e595f379720ac0fc8225db1cd999fe31e76cfa Mon Sep 17 00:00:00 2001 From: Daniel Sonck Date: Thu, 13 Jan 2022 21:44:22 +0100 Subject: [PATCH 3/5] Sort tie-breaker with requested direction When the direction is changed of the sort to descending, also apply this to the tie-breaker based on room_id. This makes the whole list flip as would be expected. Signed-off-by: Daniel Sonck --- synapse/storage/databases/main/room.py | 18 ++++++------ tests/rest/admin/test_room.py | 39 +++++++++++++------------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/synapse/storage/databases/main/room.py b/synapse/storage/databases/main/room.py index 3f6fd81b3e65..95167116c953 100644 --- a/synapse/storage/databases/main/room.py +++ b/synapse/storage/databases/main/room.py @@ -551,24 +551,24 @@ async def get_rooms_paginate( FROM room_stats_state state INNER JOIN room_stats_current curr USING (room_id) INNER JOIN rooms USING (room_id) - %s - ORDER BY %s %s, state.room_id ASC + {where} + ORDER BY {order_by} {direction}, state.room_id {direction} LIMIT ? OFFSET ? - """ % ( - where_statement, - order_by_column, - "ASC" if order_by_asc else "DESC", + """.format( + where=where_statement, + order_by=order_by_column, + direction="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 + {where} ) AS get_room_ids - """ % ( - where_statement, + """.format( + where=where_statement, ) def _get_rooms_paginate_txn( diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 1aff3626367c..f56d16fe349b 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -1363,8 +1363,10 @@ def _order_test( room_id_3 = self.helper.create_room_as(self.admin_user, tok=self.admin_user_tok) # Also create a list sorted by IDs for properties that are equal (and thus sorted by room_id) - all_ids = [room_id_1, room_id_2, room_id_3] - all_ids.sort() + sorted_by_room_id_asc = [room_id_1, room_id_2, room_id_3] + sorted_by_room_id_asc.sort() + sorted_by_room_id_desc = sorted_by_room_id_asc.copy() + sorted_by_room_id_desc.reverse() # Set room names in alphabetical order. room 1 -> A, 2 -> B, 3 -> C self.helper.send_state( @@ -1419,31 +1421,30 @@ def _order_test( "joined_local_members", [room_id_1, room_id_2, room_id_3], reverse=True ) - _order_test("version", all_ids) - _order_test("version", all_ids, reverse=True) + _order_test("version", sorted_by_room_id_asc, reverse=True) + _order_test("version", sorted_by_room_id_desc) - _order_test("creator", all_ids) - _order_test("creator", all_ids, reverse=True) + _order_test("creator", sorted_by_room_id_asc) + _order_test("creator", sorted_by_room_id_desc, reverse=True) - _order_test("encryption", all_ids) - _order_test("encryption", all_ids, reverse=True) + _order_test("encryption", sorted_by_room_id_asc) + _order_test("encryption", sorted_by_room_id_desc, reverse=True) - _order_test("federatable", all_ids) - _order_test("federatable", all_ids, reverse=True) + _order_test("federatable", sorted_by_room_id_asc) + _order_test("federatable", sorted_by_room_id_desc, reverse=True) - _order_test("public", all_ids) - # Different sort order of SQlite and PostreSQL - # _order_test("public", [room_id_3, room_id_2, room_id_1], reverse=True) + _order_test("public", sorted_by_room_id_asc) + _order_test("public", sorted_by_room_id_desc, reverse=True) - _order_test("join_rules", all_ids) - _order_test("join_rules", all_ids, reverse=True) + _order_test("join_rules", sorted_by_room_id_asc) + _order_test("join_rules", sorted_by_room_id_desc, reverse=True) - _order_test("guest_access", all_ids) - _order_test("guest_access", all_ids, reverse=True) + _order_test("guest_access", sorted_by_room_id_asc) + _order_test("guest_access", sorted_by_room_id_desc, reverse=True) - _order_test("history_visibility", all_ids) + _order_test("history_visibility", sorted_by_room_id_asc) _order_test( - "history_visibility", all_ids, reverse=True + "history_visibility", sorted_by_room_id_desc, reverse=True ) _order_test("state_events", [room_id_3, room_id_2, room_id_1]) From 93a1ecfa4fc98ca3e0e9b6aeb16fafa123cab5e0 Mon Sep 17 00:00:00 2001 From: Daniel Sonck Date: Thu, 13 Jan 2022 21:58:58 +0100 Subject: [PATCH 4/5] Fix codestyle for "history_visibility". Line can be on 1 line due to sorted_by_room_id_desc being shorter than the array. Signed-off-by: Daniel Sonck --- tests/rest/admin/test_room.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index f56d16fe349b..924755d3df28 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -1443,9 +1443,7 @@ def _order_test( _order_test("guest_access", sorted_by_room_id_desc, reverse=True) _order_test("history_visibility", sorted_by_room_id_asc) - _order_test( - "history_visibility", sorted_by_room_id_desc, reverse=True - ) + _order_test("history_visibility", sorted_by_room_id_desc, reverse=True) _order_test("state_events", [room_id_3, room_id_2, room_id_1]) _order_test("state_events", [room_id_1, room_id_2, room_id_3], reverse=True) From 4d15a5df2f4bfe5bd2306d514edea91b33456092 Mon Sep 17 00:00:00 2001 From: Daniel Sonck Date: Fri, 14 Jan 2022 19:39:08 +0100 Subject: [PATCH 5/5] Document different order direction of numerical fields "joined_members", "joined_local_members", "version" and "state_events" are ordered in descending direction by default (dir=f). Added a note in tests to explain the differences in ordering. Signed-off-by: Daniel Sonck --- tests/rest/admin/test_room.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/rest/admin/test_room.py b/tests/rest/admin/test_room.py index 924755d3df28..3495a0366ad3 100644 --- a/tests/rest/admin/test_room.py +++ b/tests/rest/admin/test_room.py @@ -1413,14 +1413,17 @@ def _order_test( _order_test("canonical_alias", [room_id_1, room_id_2, room_id_3]) _order_test("canonical_alias", [room_id_3, room_id_2, room_id_1], reverse=True) + # Note: joined_member counts are sorted in descending order when dir=f _order_test("joined_members", [room_id_3, room_id_2, room_id_1]) _order_test("joined_members", [room_id_1, room_id_2, room_id_3], reverse=True) + # Note: joined_local_member counts are sorted in descending order when dir=f _order_test("joined_local_members", [room_id_3, room_id_2, room_id_1]) _order_test( "joined_local_members", [room_id_1, room_id_2, room_id_3], reverse=True ) + # Note: versions are sorted in descending order when dir=f _order_test("version", sorted_by_room_id_asc, reverse=True) _order_test("version", sorted_by_room_id_desc) @@ -1445,6 +1448,7 @@ def _order_test( _order_test("history_visibility", sorted_by_room_id_asc) _order_test("history_visibility", sorted_by_room_id_desc, reverse=True) + # Note: state_event counts are sorted in descending order when dir=f _order_test("state_events", [room_id_3, room_id_2, room_id_1]) _order_test("state_events", [room_id_1, room_id_2, room_id_3], reverse=True)