Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Make pagination of rooms in admin api stable #11737

Merged
merged 5 commits into from
Jan 17, 2022
Merged

Make pagination of rooms in admin api stable #11737

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
942a055
Make pagination of rooms in admin api stable
Jan 13, 2022
2fdeced
Fix failing tests
dsonck92 Jan 13, 2022
d2e595f
Sort tie-breaker with requested direction
dsonck92 Jan 13, 2022
93a1ecf
Fix codestyle for "history_visibility".
dsonck92 Jan 13, 2022
4d15a5d
Document different order direction of numerical fields
dsonck92 Jan 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions synapse/storage/databases/main/room.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
DMRobertson marked this conversation as resolved.
Show resolved Hide resolved
)

# 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(
Expand Down
39 changes: 20 additions & 19 deletions tests/rest/admin/test_room.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -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)
Comment on lines +1427 to +1428
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea why this requires the reverse to be swapped. It was required locally, maybe CI tests differently, but I cannot explain it since all rooms should be the same version, and reversing works the same (and correctly) for the rest of the test cases

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's odd. Thanks for flagging it up; let's see what CI says and maybe we can investigate in more detail later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I think I see it now. The initial sort order is controlled by order_by_asc, which differs between different sorting orders. See e.g.:

synapse/synapse/storage/databases/main/room.py

Lines 506 to 511 in d2e595f

elif RoomSortOrder(order_by) == RoomSortOrder.VERSION:
order_by_column = "rooms.room_version"
order_by_asc = False
elif RoomSortOrder(order_by) == RoomSortOrder.CREATOR:
order_by_column = "rooms.creator"
order_by_asc = True

dir=b reverses this rather than setting it to False.

I guess the idea is to use the "natural" sort direction by default while still allowing the user to reverse it. Not sure if I agree with that choice, but it looks like the behaviour will be consistent.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(This goes back to #6720)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the idea is to use the "natural" sort direction by default while still allowing the user to reverse it.

That indeed was the intention, and back then there were only two sort orders - size of joined members and alphabetical room names. It seemed sensible for users to want to view rooms sorted by size of joined members descending, whereas users would want to see room names sorted from a->z.

Of course, client software could just set the dir=b flag itself when sorting alphabetically, but that's extra work for the client. Default directions are noted in the docs.

I'm just coming into this conversation without much context... but I fail to see why setting the direction order based on ordering type is confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I fail to see why setting the direction order based on ordering type is confusing.

In a nutshell, we have different interpretations of what dir=b ought to mean

  • as-implemented: reverse the natural ordering
  • our guess: sort the results descending by the given criterion

Copy link
Contributor Author

@dsonck92 dsonck92 Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aah I see, yes, in my case this will end up confusing:

image

Because the arrow directly translates to b or f (as usually done through APIs, but usually asc desc where meaning is well defined), and there is some meaning behind the arrow. So now, blindly translating the direction to b or f will give the wrong arrow direction for some column sortings. This could be fixed client-side by keeping track of the natural way of sorting, but then the logic becomes more complex and based on how it's written in Synapse. (One could argue it's already tightly bound to synapse as it works with this specific API, but it makes the sorting non-trivial)

Copy link
Contributor Author

@dsonck92 dsonck92 Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I feel like the initial sort direction is the choice of the client, and the backend just returns exactly what you ask it. That way, the client can be written knowing exactly what it will return without looking at the docs/source code and without having some lookup table to correctly swap arrows based on field.

Because if it does need a translation table, at that stage, you're essentially doing double work: backend is opinionated about sorting direction, so will pick a (well defined, but seemingly arbitrary) direction that can be flipped, and client compensates for this opinion by swapping the arrow to match the direction.

If it's not doing the opinionated default direction, then there's a lot less lines necessary in the backend (since it's always asc or desc, unless flipped), and no logic in the client necessary to compensate for it.

But! That's my opinion. I have to check with the existing synapse-admin to see if it actually takes this into account.

Edit: No, it simply forwards as well. I would say, I can finish this PR based on current behavior and add the comments. Later it can be decided whether it's important.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the discussion both - I've ended up coming to the same conclusion. It is unnecessary to maintain this "lookup table" on both ends.

I've created a separate issue to track this at #11759.


_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)

dsonck92 marked this conversation as resolved.
Show resolved Hide resolved
_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])
Expand Down