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

Search in columns 'name' and 'displayname' in the admin users endpoint #7377

Merged
merged 8 commits into from
Aug 25, 2020
Merged

Search in columns 'name' and 'displayname' in the admin users endpoint #7377

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bed8190
Search in columns 'name' and 'displayname' in the admin users endpoint
awesome-manuel Apr 30, 2020
3970339
Rename filter parameter from 'user_id' to 'name'
awesome-manuel Jun 10, 2020
06004a5
Support legacy parameter 'user_id'
awesome-manuel Jun 15, 2020
2468621
Merge remote-tracking branch 'upstream/develop' into admin_users
awesome-manuel Jun 19, 2020
e699f7e
Merge remote-tracking branch 'upstream/develop' into admin_users
awesome-manuel Aug 24, 2020
ae5477c
Keep functionality of legacy parameter 'user_id'
awesome-manuel Aug 24, 2020
42bcd0b
Use suggested SQL pattern
awesome-manuel Aug 25, 2020
7ea27cd
Fix SQL pattern
awesome-manuel Aug 25, 2020
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
1 change: 1 addition & 0 deletions changelog.d/7377.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Search in columns 'name' and 'displayname' in the admin users endpoint. Contributed by Awesome Technologies Innovationslabor GmbH.
2 changes: 1 addition & 1 deletion docs/admin_api/user_admin_api.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ The parameter ``limit`` is optional but is used for pagination, denoting the
maximum number of items to return in this call. Defaults to ``100``.

The parameter ``user_id`` is optional and filters to only users with user IDs
anoadragon453 marked this conversation as resolved.
Show resolved Hide resolved
that contain this value.
or display name that contain this value.
Copy link
Member

Choose a reason for hiding this comment

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

Mm, actually I talked to some of the other devs and we agree that changing the behaviour of user_id from under users isn't the best course of action. Additionally, if someone would like to only filter by user_id and not displayname, they'd be unable to do so anymore.

Can we add a new field called user_id_or_displayname which searches both fields? I agree that it's not the prettiest name but it should do what you desire without implementing complex OR functionality for all fields in the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe just name to search in both? Anyway a problem with searching in the user_id still is that it also searches inside the domain part. So if you domain has some very common character sequence you will get a lot of users.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just name to search in both?

This is fine. I'm a little weary that we call the column name in the users table in Synapse which holds User IDs, but that's just an internal detail. If the docs describe it as both user IDs and displaynames then it should be fine.

Anyway a problem with searching in the user_id still is that it also searches inside the domain part. So if you domain has some very common character sequence you will get a lot of users.

You could add another field just for displayname, or are you proposing one for localpart?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My personal opinion is that we would not add separate search fields in the admin UI, so a single query parameter is enough for us.
Searching in both, user_id and displayname at the same time just gives you a few more results than expected in some cases. It will definitely never be precise enough to select exactly everyone with a specific surname.
For the user_id I found a solution to only search inside the localpart, see updated PR.


The parameter ``guests`` is optional and if ``false`` will **exclude** guest users.
Defaults to ``true`` to include guest users.
Expand Down
28 changes: 16 additions & 12 deletions synapse/storage/data_stores/main/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ def get_users_paginate(
Args:
start (int): start number to begin the query from
limit (int): number of rows to retrieve
name (string): filter for user names
name (string): search for user_id or display name
guests (bool): whether to in include guest users
deactivated (bool): whether to include deactivated users
Returns:
Expand All @@ -520,11 +520,11 @@ def get_users_paginate(

def get_users_paginate_txn(txn):
filters = []
args = []
args = [self.hs.config.server_name]

if name:
filters.append("name LIKE ?")
args.append("%" + name + "%")
filters.append("(name LIKE ? OR displayname LIKE ?)")
args.extend(["%" + name + "%:%", "%" + name + "%"])

if not guests:
filters.append("is_guest = 0")
Expand All @@ -534,22 +534,26 @@ def get_users_paginate_txn(txn):

where_clause = "WHERE " + " AND ".join(filters) if len(filters) > 0 else ""

sql = "SELECT COUNT(*) as total_users FROM users %s" % (where_clause)
txn.execute(sql, args)
count = txn.fetchone()[0]

args = [self.hs.config.server_name] + args + [limit, start]
sql = """
SELECT name, user_type, is_guest, admin, deactivated, displayname, avatar_url
sql_base = """
FROM users as u
LEFT JOIN profiles AS p ON u.name = '@' || p.user_id || ':' || ?
{}
ORDER BY u.name LIMIT ? OFFSET ?
""".format(
where_clause
)
sql = "SELECT COUNT(*) as total_users " + sql_base
txn.execute(sql, args)
count = txn.fetchone()[0]

sql = (
"SELECT name, user_type, is_guest, admin, deactivated, displayname, avatar_url "
+ sql_base
+ " ORDER BY u.name LIMIT ? OFFSET ?"
)
args += [limit, start]
txn.execute(sql, args)
users = self.db.cursor_to_dict(txn)

return users, count

return self.db.runInteraction("get_users_paginate_txn", get_users_paginate_txn)
Expand Down