Add order_by to list users' media admin API

[dklimpel]

Add order_by to the admin API GET /_synapse/admin/v1/users/<user_id>/media.
This is a breaking change for default sort order to #8647 which was introduced in Synapse v1.23.0.

It is possible to prevent the breaking change.
At the moment ist the sort per default ascending.
In the room API has every order field its own default direction:

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

Lines 391 to 438 in 4218473

	# Set ordering
	if RoomSortOrder(order_by) == RoomSortOrder.SIZE:
	# Deprecated in favour of RoomSortOrder.JOINED_MEMBERS
	order_by_column = "curr.joined_members"
	order_by_asc = False
	elif RoomSortOrder(order_by) == RoomSortOrder.ALPHABETICAL:
	# Deprecated in favour of RoomSortOrder.NAME
	order_by_column = "state.name"
	order_by_asc = True
	elif RoomSortOrder(order_by) == RoomSortOrder.NAME:
	order_by_column = "state.name"
	order_by_asc = True
	elif RoomSortOrder(order_by) == RoomSortOrder.CANONICAL_ALIAS:
	order_by_column = "state.canonical_alias"
	order_by_asc = True
	elif RoomSortOrder(order_by) == RoomSortOrder.JOINED_MEMBERS:
	order_by_column = "curr.joined_members"
	order_by_asc = False
	elif RoomSortOrder(order_by) == RoomSortOrder.JOINED_LOCAL_MEMBERS:
	order_by_column = "curr.local_users_in_room"
	order_by_asc = False
	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
	elif RoomSortOrder(order_by) == RoomSortOrder.ENCRYPTION:
	order_by_column = "state.encryption"
	order_by_asc = True
	elif RoomSortOrder(order_by) == RoomSortOrder.FEDERATABLE:
	order_by_column = "state.is_federatable"
	order_by_asc = True
	elif RoomSortOrder(order_by) == RoomSortOrder.PUBLIC:
	order_by_column = "rooms.is_public"
	order_by_asc = True
	elif RoomSortOrder(order_by) == RoomSortOrder.JOIN_RULES:
	order_by_column = "state.join_rules"
	order_by_asc = True
	elif RoomSortOrder(order_by) == RoomSortOrder.GUEST_ACCESS:
	order_by_column = "state.guest_access"
	order_by_asc = True
	elif RoomSortOrder(order_by) == RoomSortOrder.HISTORY_VISIBILITY:
	order_by_column = "state.history_visibility"
	order_by_asc = True
	elif RoomSortOrder(order_by) == RoomSortOrder.STATE_EVENTS:
	order_by_column = "curr.current_state_events"
	order_by_asc = False

In my opinion this makes it complicated because you don't know what the default sort order is now.

Related to: #8647 (comment)

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)

Signed-off-by: Dirk Klimpel dirk@klimpel.org

[clokep]

It seems like it would be pretty straightforward to make the default the previous sort order, which would make it backwards compatible. Is there a good reason not to do this?

[dklimpel]

It seems like it would be pretty straightforward to make the default the previous sort order, which would make it backwards compatible. Is there a good reason not to do this?

I have did an update to change default sort order to prevent breaking change.

[erikjohnston]

I think this looks OK, my concern is the performance of this. We only have indexes on media_id, user_id and created_ts, so the other sort fields will involve walking the entire table. I don't know if we should a) only support sorting by those columns or b) put warnings in the doc?

[clokep]

@dklimpel Is this ready for another look?

[dklimpel]

@dklimpel Is this ready for another look?

Yes.

[richvdh]

looks generally great! A few suggestions here.

[richvdh]

I think it would be clearer to do:

Suggested change

	if (
	parse_string(request, "order_by") is None
	and parse_string(request, "dir") is None
	):
	order_by = MediaSortOrder.CREATED_TS.value
	direction = "b"
	if (
	"order_by" not in request.args and "dir" not in request.args
	):
	order_by = MediaSortOrder.CREATED_TS.value
	direction = "b"
	else:
	order_by = parse_string(...) # etc

[clokep]

Looks pretty good overall, but I have a couple of comments, sorry about that!

[clokep]

Isn't this identical to providing the default key below?

[dklimpel]

This is for the special case of backward compatibility of this API. In case of no request parameter there is a special value of default: direction is b. In a normal case the direction is f.
It was a request: #8978 (comment)

[clokep]

Ah, right! I missed that the direction was opposite the default. Good catch!

[clokep]

I think this block isn't really necessary and can be replaced with the following:

order_by_column = MediaSortOrder(order_by).value

This will raise a ValueError if an invalid value is provided.

[clokep]

This seems unfortunate. Why is this not the case for safe_from_quarantine?

[dklimpel]

I will do a new test.

[dklimpel]

Different sort order of SQlite and PostreSQL:
If a media is not in quarantine quarantined_by is NULL
SQLite considers NULL to be smaller than any other value.
PostreSQL considers NULL to be larger than any other value.

[clokep]

I think this is OK. I'm not thrilled about the sort order being different from sqlite and postgresql, but I don't know if there's a solution to that. (I'll leave this for a day or two before merging in case someone else has an idea though.)

changes handled

[dklimpel]

I think this is OK. I'm not thrilled about the sort order being different from sqlite and postgresql, but I don't know if there's a solution to that. (I'll leave this for a day or two before merging in case someone else has an idea though.)

If someone has an idea. Some information.

synapse/synapse/storage/databases/main/search.py

Lines 569 to 579 in 1baab20

	# We add an arbitrary limit here to ensure we don't try to pull the
	# entire table from the database.
	if isinstance(self.database_engine, PostgresEngine):
	sql += (
	" ORDER BY origin_server_ts DESC NULLS LAST,"
	" stream_ordering DESC NULLS LAST LIMIT ?"
	)
	elif isinstance(self.database_engine, Sqlite3Engine):
	sql += " ORDER BY origin_server_ts DESC, stream_ordering DESC LIMIT ?"
	else:
	raise Exception("Unrecognized database engine")

https://www.postgresql.org/docs/current/queries-order.html
The NULLS FIRST and NULLS LAST options can be used to determine whether nulls appear before or after non-null values in the sort ordering. By default, null values sort as if larger than any non-null value; that is, NULLS FIRST is the default for DESC order, and NULLS LAST otherwise.

https://www.sqlitetutorial.net/sqlite-order-by/
SQLite 3.30.0 added the NULLS FIRST and NULLS LAST options to the ORDER BY clause. The NULLS FIRST option specifies that the NULLs will appear at the beginning of the result set while the NULLS LAST option place NULLs at the end of the result set.

It seems that with the same syntax.
What is the best solution?
ASC NULLS LAST and DESC NULLS FIRST
or
ASC NULLS FIRST and DESC NULLS LAST

See also: https://learnsql.com/blog/how-to-order-rows-with-nulls/

[clokep]

From chatting about this with the team it seems there's a few other spots where the ordering differs between SQLite and PostgreSQL. So I think this is OK.