Make pagination of rooms in admin api stable

[dsonck92]

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)

Fixes: #11736

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[DMRobertson]

Good spot. An oversight in #6720?

A few questions:

• What will happen if the request asks to sort by room name descending? Then we'll end up doing ... ORDER BY state.name DESC, state.name ASC ... which looks like it might be an invalid query? Postgres seems happy with this though.
• This won't be stable if a room's name changes between pagination queries? (I don't think we have a good way to fix this. Could tie-break on room_id, but that's not stable either, because new room ids get created all the time.)

I think the tests for this endpoint might need tweaking too. I've just seen that this is marked as a draft, so I guess you're probably working on this now? Either way, let us know if you could use help!

[dsonck92]

I actually realized later that it was wiser to sort on room_id instead, name is not necessarily unique. I just realized that I might have kept the old commit message (which was also used as auto-fill by github).

Yes, I do agree that sorting on room_id may shift results as rooms come and go, however, the current implementation kinda leaves it up to implementation details how the sort will become. In PostgreSQL (at least), the implicit order is mostly defined by insertion and touching order. However, as I worked through my own Synapse Admin derivative, the current implementation can show a room on 3 different pages. If it's based on room_id, then, sure, maybe a room or two get added, so rooms may be duplicated if they happened before the current page, but it doesn't completely throw out the previous sort.

[dsonck92]

And yes, small oversight in that commit. I also checked the user list endpoint, but this one does have a fallback. I'm indeed fixing the tests because, apparently they magically always worked (probably because the table mostly responsible for testing was never touched afterwards, keeping the implicit ordering). But, I can push soon as tests work again!

[dsonck92]

Regarding a true stable way, coming from GraphQL where there's the opinionated Relay spec, this could potentially be fixed by cursors. In essence, instead of relying on, say, a page offset, you actually encode some cursor data to use to resume. For instance, if you sort on room_id, you can also compare room_id and select anything > last room_id. This way, if you have more items inserted before the current page, this cursor moves along, and the client would only notice the total amount of results changing. However, for the opposite direction, it's more annoying to do (likely some "earlier, swap implicit sort, limit by page, reorder for client"). Not to mention calculating the offset as feedback for which page you're on, and the fact that you now skipped maybe 1.1 pages instead of exactly 1 or 2.

I don't think it's that critical to consider implementing it.

[dsonck92]

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

[DMRobertson]

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

[DMRobertson]

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.

[DMRobertson]

(This goes back to #6720)

[anoadragon453]

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.

[DMRobertson]

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

[dsonck92]

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

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)

[dsonck92]

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.

[anoadragon453]

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.

[DMRobertson]

Could you leave a comment above the order test for "version" explaining what's going on? Something like # Note: versions are sorted in descending order when dir=f perhaps?

The other sort orders that behave in this way are "joined_members", "joined_local_members" and "state_events". Those all have explicit tests that don't rely on the tie-breaker mechanism, but perhaps similar comments would be worthwhile? I'll leave that to your discretion.

Once that's done I think this is good to go!

[DMRobertson]

LGTM. Thanks for seeing this through!