-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Manually search for displayname by iterating over group members #27531
Conversation
304292c
to
e0ef680
Compare
The only backend I found when checking again that does not support search would be guests: |
Should we add it there and/or make it mandatory for group backends to support? Maybe we can provide a fallback in ABackend calling usersInGroup and filtering. |
@juliushaertl This is supplanted by #32866 no? |
Still seems like a separate optimization as the search call over all groups with displayNamesInGroup is not touched in #32866 But I'd say this should be base on the new searchInGroup method then with the ISearchableGroupBackend.php interface |
e0ef680
to
74bc46c
Compare
Signed-off-by: Julius Härtl <[email protected]>
74bc46c
to
78970ce
Compare
foreach ($filteredUsers as $filteredUser) { | ||
if ($group->inGroup($filteredUser)) { | ||
$groupUsers[] = $filteredUser; | ||
$userChunk = $group->searchUsers('', $chunkLimit, $chunkOffset); |
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.
As I understand it, searchUsers
allows to filter by uid and display name.
I guess we could filter a bit with it:
$userChunk = $group->searchUsers('', $chunkLimit, $chunkOffset); | |
$userChunk = $group->searchUsers($search, $chunkLimit, $chunkOffset); |
I encountered that searching for displaynames within a group is quite bad performance wise as the it basically performs inGroup check for all users. A possible alternative would be to manually get the group users and do the filtering on the displayname manually. In my understanding this should give the same results at least as the database backend which just does an ILIKE '%search%' wich no placeholder support in the search term itself. @blizzz Do you see any issues with that for LDAP for example or in general?
Especially with large user directories and groupfolders with multiple groups assigned this could bring quite a performance boost, compared to the previous one when searching for those users (see example comparison for the groupfolder user search https://blackfire.io/profiles/compare/1baf5c45-af1f-42c1-99c2-0c8be6eb777e/graph)