-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Add search by room ID and room alias to List Room admin API #11099
Changes from 2 commits
481cb3c
3970b7c
f1e92e6
2ab73a0
2451fc4
831e2ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Add search by room ID and room alias to List Room admin API. |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -409,22 +409,31 @@ async def get_rooms_paginate( | |||||||||||||||||||
limit: maximum amount of rooms to retrieve | ||||||||||||||||||||
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 | ||||||||||||||||||||
search_term: a string to filter room names, | ||||||||||||||||||||
DMRobertson marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
canonical alias and room ids by | ||||||||||||||||||||
Returns: | ||||||||||||||||||||
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 = "" | ||||||||||||||||||||
if search_term: | ||||||||||||||||||||
where_statement = "WHERE LOWER(state.name) LIKE ?" | ||||||||||||||||||||
where_statement = """ | ||||||||||||||||||||
WHERE LOWER(state.name) LIKE ? | ||||||||||||||||||||
OR LOWER(state.canonical_alias) LIKE ? | ||||||||||||||||||||
OR LOWER(state.room_id) LIKE ? | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think searching for substrings of room ids, or for case-insensitive matches of room ids, is a sensible thing to do. Room IDs aren't human-readable, and are meant to be opaque strings. It's dangerous to assume it even has a domain part - various spec-change proposals suggest changing that. If people want to search by room id, they should enter the complete room ID and we should search for exactly that room id. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||||||||||||||||
""" | ||||||||||||||||||||
|
||||||||||||||||||||
# 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.lower() + "%" | ||||||||||||||||||||
search_term = [ | ||||||||||||||||||||
dklimpel marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||
"%" + search_term.lower() + "%", | ||||||||||||||||||||
"#%" + search_term.lower() + "%:%", | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this feels a bit redundant. All canonical aliases should start with # and contain a domain part, so why not just:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason/idea is to search only in local part and not in server part. synapse/synapse/storage/databases/main/__init__.py Lines 289 to 295 in 66bdca3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I'm not entirely sure we should only search the local part here, but ok. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can change it, if you like. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fair point! |
||||||||||||||||||||
"!%" + search_term.lower() + "%:%", | ||||||||||||||||||||
] | ||||||||||||||||||||
|
||||||||||||||||||||
# Set ordering | ||||||||||||||||||||
if RoomSortOrder(order_by) == RoomSortOrder.SIZE: | ||||||||||||||||||||
|
@@ -517,10 +526,10 @@ async def get_rooms_paginate( | |||||||||||||||||||
|
||||||||||||||||||||
def _get_rooms_paginate_txn(txn): | ||||||||||||||||||||
# Execute the data query | ||||||||||||||||||||
sql_values = (limit, start) | ||||||||||||||||||||
sql_values = [limit, start] | ||||||||||||||||||||
if search_term: | ||||||||||||||||||||
# Add the search term into the WHERE clause | ||||||||||||||||||||
sql_values = (search_term,) + sql_values | ||||||||||||||||||||
sql_values = search_term + sql_values | ||||||||||||||||||||
txn.execute(info_sql, sql_values) | ||||||||||||||||||||
|
||||||||||||||||||||
# Refactor room query data into a structured dictionary | ||||||||||||||||||||
|
@@ -548,7 +557,7 @@ def _get_rooms_paginate_txn(txn): | |||||||||||||||||||
# Execute the count query | ||||||||||||||||||||
|
||||||||||||||||||||
# Add the search term into the WHERE clause if present | ||||||||||||||||||||
sql_values = (search_term,) if search_term else () | ||||||||||||||||||||
sql_values = search_term if search_term else () | ||||||||||||||||||||
txn.execute(count_sql, sql_values) | ||||||||||||||||||||
|
||||||||||||||||||||
room_count = txn.fetchone() | ||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a bit tricky to parse the and/or logic here. I didn't understand at first that we had to match within the local part of the room id: I read
local part of canonical alias or room id
as(local part of canonical alias) or room id
.Perhaps something like this might be clearer? (Not a hard suggestion, feel free to tweak)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels really weird to me that we would re-use this for room ID. If we have the room ID can't we just use it directly?
I think it would make more sense to make the room details API take a room ID or alias?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use a CLI it is right. But if you have an GUI like Synapse-Admin, you have to filter your room from the list and after this you can select the room for details.
And you get an API similar to user API: https://matrix-org.github.io/synapse/latest/admin_api/user_admin_api.html#list-accounts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see both sides here. As @dklimpel says this is probably helpful for GUI apps. But, per #11099 (comment): I do think we shouldn't be attempting substring matching on room IDs